Try to understand compiler logic about static vars/funcs

Go To Last Post
21 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello, dear community.

 

I try to optimize (and secure a bit) my source code. I have main module with loop and some other modules which are responsible for doing different tasks - TWI I/O, printing on 7-segment LED display etc. I use avr-gcc 9.2.0 and the following Makefile:

CC = avr-gcc
OBJCOPY = avr-objcopy
SIZE = avr-size
NM = avr-nm
AVRDUDE = avrdude
REMOVE = rm -f

MCU = atmega8a
F_CPU = 8000000

LFUSE = 0x9f
HFUSE = 0xd1

TARGET = main
SRC = $(TARGET).c bme280.c led.c twi.c
OBJ = $(SRC:.c=.o)
OUTPUT = flash

FORMAT = ihex

OPTLEVEL = s

CDEFS = 

CFLAGS = -DF_CPU=$(F_CPU)UL
CFLAGS += $(CDEFS)
CFLAGS += -O$(OPTLEVEL)
CFLAGS += -mmcu=$(MCU)
CFLAGS += -std=gnu99
CFLAGS += -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums
CFLAGS += -ffunction-sections -fdata-sections
CFLAGS += -Wall -Wstrict-prototypes
CFLAGS += -Wa,-adhlns=$(<:.c=.lst)

LDFLAGS = -Wl,--gc-sections

AVRDUDE_MCU = atmega8
AVRDUDE_PROGRAMMER = usbasp
AVRDUDE_SPEED = -B 1MHz

AVRDUDE_FLAGS = -p $(AVRDUDE_MCU)
AVRDUDE_FLAGS += -c $(AVRDUDE_PROGRAMMER)
AVRDUDE_FLAGS += $(AVRDUDE_SPEED)

MSG_LINKING = Linking:
MSG_COMPILING = Compiling:
MSG_FLASH = Preparing HEX file:

all: gccversion $(TARGET).elf $(TARGET).hex size

.SECONDARY: $(TARGET).elf
.PRECIOUS: $(OBJ)

%.hex: %.elf
	@echo
	@echo $(MSG_FLASH) $@
	$(OBJCOPY) -O $(FORMAT) -j .text -j .data $< $@

%.elf: $(OBJ)
	@echo
	@echo $(MSG_LINKING) $@
	$(CC) -mmcu=$(MCU) $(LDFLAGS) $^ --output $(@F)

%.o : %.c
	@echo $(MSG_COMPILING) $<
	$(CC) $(CFLAGS) -c $< -o $(@F)

gccversion:
	@$(CC) --version

size: $(TARGET).elf
	@echo
	$(SIZE) -C --mcu=$(AVRDUDE_MCU) $(TARGET).elf

analyze: $(TARGET).elf
	$(NM) -S --size-sort -t decimal $(TARGET).elf

isp: $(TARGET).hex
	$(AVRDUDE) $(AVRDUDE_FLAGS) -U flash:w:$(TARGET).hex

fuses:
	$(AVRDUDE) $(AVRDUDE_FLAGS) -U lfuse:w:$(LFUSE):m -U hfuse:w:$(HFUSE):m

release: fuses isp

clean:
	$(REMOVE) $(TARGET).hex $(TARGET).elf $(OBJ) *~

I want to keep only necessary declarations/defines/constants in my header files and mark all internal routines and service variables as static inside corresponding source files. Let's take for example the following - module for getting data from BME280 sensor.

 

bme280.h

#ifndef _BME280_H
#define _BME280_H

#include <stdint.h>

/* TODO add support for BMP280 and unify all functions */
/* registers addresses */
#define BME280_ID_ADDR                      0xD0
#define BME280_RESET_ADDR                   0xE0
#define BME280_TP_CALIB_START_ADDR          0x88
#define BME280_HUM_H1_CALIB_START_ADDR      0xA1
#define BME280_HUM_REST_CALIB_START_ADDR    0xE1
#define BME280_CTRL_HUM_ADDR                0xF2
#define BME280_CTRL_MEAS_ADDR               0xF4
#define BME280_STATUS_ADDR                  0xF3
#define BME280_TPH_START_ADDR               0xF7
#define BME280_PRESS_START_ADDR             0xF7
#define BME280_TEMP_START_ADDR              0xFA
#define BME280_HUM_START_ADDR               0xFD

/* lengths of data registers */
#define BME280_TP_CALIB_LENGTH          24
#define BME280_H_CALIB_LENGTH           8
#define BME280_HUM_REST_CALIB_LENGTH    7
#define BME280_TPH_LENGTH               8
#define BME280_PRESS_LENGTH             3
#define BME280_TEMP_LENGTH              3
#define BME280_HUM_LENGTH               2

/* some influental parameters */
#define BME280_ID           0x60
#define BME280_I2C_ADDR     0x76

/* available commands and corresponding codes */
#define BME280_RESET_CMD    0xB6

/* limits for raw data */
#define BME280_TEMP_LOW     -4000
#define BME280_TEMP_HIGH    8500
#define BME280_PRESS_LOW    30000
#define BME280_PRESS_HIGH   110000
#define BME280_HUM_LOW      0
#define BME280_HUM_HIGH     102400

/* error codes */
#define BME280_ERR_NO       0
#define BME280_ERR_CONN     1
#define BME280_ERR_ID       2
#define BME280_ERR_CALIB    3
#define BME280_ERR_ACQ      4
#define BME280_ERR_STATUS   5
#define BME280_ERR_IO       6

extern unsigned char BME280_measure(int16_t *T, uint32_t *P, uint32_t *H, unsigned char *T_alarm, unsigned char *P_alarm, unsigned char *H_alarm); /* get readings from sensor */

#endif

bme280.c

#include "bme280.h"
#include "twi.h"

#include <stdint.h>

/* calibration coefficients */
typedef struct {
    uint16_t T1;
    int16_t T2;
    int16_t T3;

    uint16_t P1;
    int16_t P2;
    int16_t P3;
    int16_t P4;
    int16_t P5;
    int16_t P6;
    int16_t P7;
    int16_t P8;
    int16_t P9;

    unsigned char H1;
    int16_t H2;
    unsigned char H3;
    int16_t H4;
    int16_t H5;
    char H6;
} BME280_coeffs_t;

/* raw sensor data */
typedef struct {
    unsigned char t0;
    unsigned char t1;
    unsigned char t2;
    unsigned char p0;
    unsigned char p1;
    unsigned char p2;
    unsigned char h0;
    unsigned char h1;
} BME280_data_t;

BME280_coeffs_t BME280_calib;
BME280_data_t BME280_tph;

uint8_t BME280_read_id(unsigned char *id);
uint8_t BME280_reset(void);
uint8_t BME280_read_calibration(void);
uint8_t BME280_set_acquisition(const unsigned char os_t, const unsigned char os_p, const unsigned char os_h, const unsigned char mode);
uint8_t BME280_read_status(unsigned char *status);
uint8_t BME280_read_TPH(void);
void    BME280_compensate(int16_t *final_T, uint32_t *final_P, uint32_t *final_H, unsigned char *alarm_T, unsigned char *alarm_P, unsigned char *alarm_H);

/* read sensor ID */
uint8_t BME280_read_id(unsigned char *id) {
    *id = 0;

    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_ID_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_byte(id))
        return 0;

    TWI_stop();

    return 1;
}

/* issue reset */
uint8_t BME280_reset(void) {
    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_RESET_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_RESET_CMD))
        return 0;

    TWI_stop();

    return 1;
}

/* read calibration coefficients */
uint8_t BME280_read_calibration(void) {
    unsigned char tp_coeffs[BME280_TP_CALIB_LENGTH];
    unsigned char h1_coeff;
    unsigned char h_coeffs[BME280_HUM_REST_CALIB_LENGTH];

    /* read T and P calibration coefficients */
    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_TP_CALIB_START_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_nbytes(tp_coeffs, BME280_TP_CALIB_LENGTH))
        return 0;

    /* read H1 calibration coefficient */
    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_HUM_H1_CALIB_START_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_byte(&h1_coeff))
        return 0;

    /* read H2-H6 calibration coefficients */
    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_HUM_REST_CALIB_START_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_nbytes(h_coeffs, BME280_HUM_REST_CALIB_LENGTH))
        return 0;

    TWI_stop();

    /* properly combine bytes to get numeric coefficients */
    BME280_calib.T1 = (((uint16_t)tp_coeffs[1]) << 8) | ((uint16_t)tp_coeffs[0]);
    BME280_calib.T2 = (((int16_t)tp_coeffs[3]) << 8) | ((int16_t)tp_coeffs[2]);
    BME280_calib.T3 = (((int16_t)tp_coeffs[5]) << 8) | ((int16_t)tp_coeffs[4]);

    BME280_calib.P1 = (((uint16_t)tp_coeffs[7]) << 8) | ((uint16_t)tp_coeffs[6]);
    BME280_calib.P2 = (((int16_t)tp_coeffs[9]) << 8) | ((int16_t)tp_coeffs[8]);
    BME280_calib.P3 = (((int16_t)tp_coeffs[11]) << 8) | ((int16_t)tp_coeffs[10]);
    BME280_calib.P4 = (((int16_t)tp_coeffs[13]) << 8) | ((int16_t)tp_coeffs[12]);
    BME280_calib.P5 = (((int16_t)tp_coeffs[15]) << 8) | ((int16_t)tp_coeffs[14]);
    BME280_calib.P6 = (((int16_t)tp_coeffs[17]) << 8) | ((int16_t)tp_coeffs[16]);
    BME280_calib.P7 = (((int16_t)tp_coeffs[19]) << 8) | ((int16_t)tp_coeffs[18]);
    BME280_calib.P8 = (((int16_t)tp_coeffs[21]) << 8) | ((int16_t)tp_coeffs[20]);
    BME280_calib.P9 = (((int16_t)tp_coeffs[23]) << 8) | ((int16_t)tp_coeffs[22]);

    BME280_calib.H1 = (unsigned char)h1_coeff;
    BME280_calib.H2 = (((int16_t)h_coeffs[1]) << 8) | ((int16_t)h_coeffs[0]);
    BME280_calib.H3 = (unsigned char)h_coeffs[2];
    BME280_calib.H4 = (((int16_t)h_coeffs[3]) << 4) | ((int16_t)(h_coeffs[4] & 0x0F));
    BME280_calib.H5 = (((int16_t)h_coeffs[5]) << 4) | ((int16_t)(h_coeffs[4] >> 4));
    BME280_calib.H6 = (char)h_coeffs[6];

    return 1;
}

/* set measurement parameters */
uint8_t BME280_set_acquisition(const unsigned char os_t, const unsigned char os_p, const unsigned char os_h, const unsigned char mode) {
    /* write parameters for humidity oversampling first */
    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_CTRL_HUM_ADDR))
        return 0;

    if (!TWI_write_byte(os_h))
        return 0;

    /* then write the rest */
    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_CTRL_MEAS_ADDR))
        return 0;

    if (!TWI_write_byte((os_t << 5) | (os_p << 2) | mode))
        return 0;

    TWI_stop();

    return 1;
}

/* read sensor status */
uint8_t BME280_read_status(unsigned char *status) {
    unsigned char s;

    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_STATUS_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_byte(&s))
        return 0;

    TWI_stop();

    *status = s & 0x09;

    return 1;
}

/* read raw measurements */
uint8_t BME280_read_TPH(void) {
    unsigned char data[BME280_TPH_LENGTH];

    if (!TWI_start())
        return 0;

    if (!TWI_slave_write(BME280_I2C_ADDR))
        return 0;

    if (!TWI_write_byte(BME280_TPH_START_ADDR))
        return 0;

    if (!TWI_rstart())
        return 0;

    if (!TWI_slave_read(BME280_I2C_ADDR))
        return 0;

    if (!TWI_read_nbytes(data, BME280_TPH_LENGTH))
        return 0;

    TWI_stop();

    BME280_tph.t0 = data[3];
    BME280_tph.t1 = data[4];
    BME280_tph.t2 = data[5];

    BME280_tph.p0 = data[0];
    BME280_tph.p1 = data[1];
    BME280_tph.p2 = data[2];

    BME280_tph.h0 = data[6];
    BME280_tph.h1 = data[7];

    return 1;
}

/* convert raw measurements data into physical values */
void BME280_compensate(int16_t *final_T, uint32_t *final_P, uint32_t *final_H, unsigned char *alarm_T, unsigned char *alarm_P, unsigned char *alarm_H) {
    int32_t t_raw = (int32_t)((((int32_t)BME280_tph.t0) << 12) | (((int32_t)BME280_tph.t1) << 4) | (((int32_t)BME280_tph.t2) >> 4));
    int32_t var1, var2, t_fine, T;
    var1 = (((t_raw >> 3) - (((int32_t)BME280_calib.T1) << 1)) * ((int32_t)BME280_calib.T2)) >> 11;
    var2 = (((((t_raw >> 4) - ((int32_t)BME280_calib.T1)) * ((t_raw >> 4) - ((int32_t)BME280_calib.T1))) >> 12) * ((int32_t)BME280_calib.T3)) >> 14;
    t_fine = var1 + var2;
    T = (t_fine * 5 + 128) >> 8;

    if ((T <= BME280_TEMP_LOW) || (T >= BME280_TEMP_HIGH)) {
        *alarm_T = 1;

        if (T <= BME280_TEMP_LOW)
            *final_T = BME280_TEMP_LOW;
        else
            *final_T = BME280_TEMP_HIGH;
    }
    else {
        *alarm_T = 0;
        *final_T = T;
    }

    int32_t p_raw = (int32_t)((((uint32_t)BME280_tph.p0) << 12) | (((uint32_t)BME280_tph.p1) << 4) | (((uint32_t)BME280_tph.p2) >> 4));
    uint32_t P;
    var1 = (((int32_t)t_fine) >> 1) - (int32_t)64000;
    var2 = (((var1 >> 2) * (var1 >> 2)) >> 11) * ((int32_t)BME280_calib.P6);
    var2 = var2 + ((var1 * ((int32_t)BME280_calib.P5)) << 1);
    var2 = (var2 >> 2) + (((int32_t)BME280_calib.P4) << 16);
    var1 = (((BME280_calib.P3 * (((var1 >> 2) * (var1 >> 2)) >> 13)) >> 3) + ((((int32_t)BME280_calib.P2) * var1) >> 1)) >> 18;
    var1 = ((((32768 + var1)) * ((int32_t)BME280_calib.P1)) >> 15);
    P = (((uint32_t)(((int32_t)1048576) - p_raw) - (var2 >> 12))) * 3125;

    if (var1 != 0) {
        if (P < 0x80000000)
            P = (P << 1) / ((uint32_t)var1);
        else
            P = (P / ((uint32_t)var1)) * 2;

        var1 = (((int32_t)BME280_calib.P9) * ((int32_t) (((P >> 3) * (P >> 3)) >> 13))) >> 12;
        var2 = (((int32_t)(P >> 2)) * ((int32_t)BME280_calib.P8)) >> 13;
        P = (uint32_t)((int32_t)P + ((var1 + var2 + BME280_calib.P7) >> 4));
    }
    else
        P = 0;

    if ((P <= BME280_PRESS_LOW) || (P >= BME280_PRESS_HIGH)) {
        *alarm_P = 1;

        if (P < BME280_PRESS_LOW)
            *final_P = BME280_PRESS_LOW;
        else
            *final_P = BME280_PRESS_HIGH;
    }
    else {
        *alarm_P = 0;
        *final_P = P;
    }

    int32_t h_raw = (int32_t)((((int32_t)BME280_tph.h0) << 8) | ((int32_t)BME280_tph.h1));
    int32_t var3, var4, var5;
    uint32_t H;
    var1 = (int32_t)t_fine - (int32_t)76800;
    var2 = h_raw << 14;
    var3 = ((int32_t)BME280_calib.H4) << 20;
    var4 = ((int32_t)BME280_calib.H5) * var1;
    var5 = ((var2 - var3 - var4 + (int32_t)16384) >> 15);
    var2 = (var1 * ((int32_t)BME280_calib.H6)) >> 10;
    var3 = (var1 * ((int32_t)BME280_calib.H3)) >> 11;
    var4 = ((var2 * (var3 + (int32_t)32768)) >> 10) + (int32_t)2097152;
    var2 = (var4 * ((int32_t)BME280_calib.H2) + 8192) >> 14;
    var3 = var5 * var2;
    var4 = ((var3 >> 15) * (var3 >> 15)) >> 7;
    var5 = var3 - ((var4 * ((int32_t)BME280_calib.H1)) >> 4);
    var5 = (var5 < 0 ? 0 : var5);
    var5 = (var5 > 419430400 ? 419430400 : var5);
    H = (uint32_t)(var5 >> 12);

    if ((H <= BME280_HUM_LOW) || (H >= BME280_HUM_HIGH)) {
        *alarm_H = 1;

        if (H <= BME280_HUM_LOW)
            *final_H = BME280_HUM_LOW;
        else
            *final_H = BME280_HUM_HIGH;
    }
    else {
        *alarm_H = 0;
        *final_H = H;
    }
}

/* perform one measurement cycle and get results */
unsigned char BME280_measure(int16_t *T, uint32_t *P, uint32_t *H, unsigned char *T_alarm, unsigned char *P_alarm, unsigned char *H_alarm) {
    unsigned char id;

    if (!BME280_read_id(&id))
        return BME280_ERR_CONN;

    if (id != BME280_ID)
        return BME280_ERR_ID;

    if (!BME280_read_calibration())
        return BME280_ERR_CALIB;

    if (!BME280_set_acquisition(0x01, 0x01, 0x01, 0x01))
        return BME280_ERR_ACQ;

    /* wait until sensor becomes idle */
    unsigned char status;

    do {
        if (!BME280_read_status(&status))
            return BME280_ERR_STATUS;
    } while (status);

    if (status == 0) {
        if (!BME280_read_TPH())
            return BME280_ERR_IO;

        BME280_compensate(T, P, H, T_alarm, P_alarm, H_alarm); /* perform conversion */

        return BME280_ERR_NO;
    }
    else
        return BME280_ERR_STATUS;
}

As for now no static keywords are used and I have the following assembler listing (see bme280.lst_.nonstatic.txt). Firmware works as intended and I have meaningful data by calling BME280_measure() from main.c

 

If I mark BME280_calib variable as static then I have the following asm listing (see bme280.lst_.static_func.txt.). Things works fine still. However, if I mark with static not a variable but a function (for example, BME280_read_id), then I get error in my main loop (specifically, the first error is BME280_ERR_CAL and then BME280_ERR_CONN). Asm listing is here (see bme280.lst_.static_var.txt).

 

Seems like gcc moves BME280_read_id function inside BME280_measure and something wents wrong. I have little experience with assembler and wonder why this is observed and how I can fix it.

 

Attachment(s): 

This topic has a solution.

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 10:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What do you expect "static" to do, when applied to a function?

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I want to limit scope of that function to translation unit. This would be helpful to evade possible problems in future (e. g. not re-declare that function elsewhere by accident)

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Making functions static often has the effect of making them in-lined.

Making functions inline often has the effect of making them faster.

Is your device not starting fast enough to allow the new in-lined version to work correctly?

 

Making function inline sometimes has the effect of changing all the register allocations.

Sometimes causing the function to run slower, and sometimes causing the program to use a register that you had (incorrectly) assumed was safe to use in ASM code.

Are you using any ASM code?  Is the new version too slow?

 

What happens if you add a noinline attribute to your static functions?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you want to limit the scope of a function, just limit where you #include the header file that declares it.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
If you want to limit the scope of a function, just limit where you #include the header file that declares it.
Err no. That is not how you limit function scope. Say you have two files that both define their own function called increment_counter(). If they both just implement this as non-static they will be global. When the linker comes to join the two .o files built from the two .c files where they were defined it will tell you "multiple definition of increment_counter". To avoid this one, other or both should be "static" so that the linker effectively cannot "see" them. The calls made to increment_counter() are each just local to the file where that and the calling invocation are located.

 

Of course if one were in adc.c and the other in timer.c then a way to implement "namespace" in plain C is simply to prepend something that ties the function to the file it is in so you then have adc_increment_counter() and timer_increment_counter() and then there is no name space pollution.

 

C++ has a much neater solution for this which is namespaces. In C++ you would give everything in the ADC file a adc:: namespace and everything in the timer file a timer:: namespace so they are actually two quite different function names: adc::increment_counter() and timer::increment_counter() but you don't always need to apply the namespace prefix if "using namespace timer" or whatever. Then a call to increment_counter() means the timer:increment_counter() function.

 

As to the original problem in this thread the crux of the matter seems to be:

dviktor wrote:
l. However, if I mark with static not a variable but a function (for example, BME280_read_id), then I get error in my main loop

To diagnose that we need to see the main loop and the exact text of the error message. The implication would seem to be that the BME280_Measure() exposed in the .h as the only global function is NOT the only globally accessed function.

 

(again C++ handles this all a lot better - you make functions public:, private:, protected: to control how far and wide they can be accessed. If a member function is private: then you simply could not access it from anywhere but code in the class.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

melbourne

no, version with static funcs simply doesn't work - error flag is returned, however without static keyword the same code works just fine

 

clawson

#include "bme280.h"
#include "led.h"
#include "twi.h"

#include <avr/io.h>
#include <avr/interrupt.h>
#include <stdint.h>
#include <util/atomic.h>
#include <util/delay.h>
#include <util/twi.h>

#define SYS_MEAS_DELAY  1 /* in tics */
#define SYS_MODE_DELAY  3 /* in seconds */

#define SYS_MIN_MODE    0
#define SYS_MAX_MODE    3

#define SYS_MODE_T      0
#define SYS_MODE_P      1
#define SYS_MODE_H      2
#define SYS_MODE_TIME   3
#define SYS_MODE_START  255

volatile uint16_t SYS_tics; /* TODO check for overflowing */

/* handle dynamic switching */
ISR(TIMER0_OVF_vect) {
    static const unsigned char fields[4] = {(1 << LED_FLD_1), (1 << LED_FLD_2), (1 << LED_FLD_3), (1 << LED_FLD_4)}; /* these fields on LED screen can be accessed */
    static uint8_t fld = LED_FLD_MIN; /* current field */

    /* shut down all fields and segments */
    LED_SEG_PORT &= ~LED_SEG_ALL;
    LED_FLD_PORT |= LED_FLD_ALL;
    /* enable current field and show corresponding symbol */
    LED_SEG_PORT |= LED_symbols[fld];
    LED_FLD_PORT &= ~fields[fld];

    /* iterate */
    if (fld == LED_FLD_MAX)
        fld = LED_FLD_MIN;
    else
        fld++;
}

/* count for internal tics */
ISR(TIMER1_COMPA_vect) {
    SYS_tics++;
}

/* prepare I/O lines */
void setup_io(void) {
    /* segments, fields and dots are outputs */
    LED_SEG_DDR |= LED_SEG_ALL;
    LED_FLD_DDR |= LED_FLD_ALL;
    LED_DDOT_DDR |= (1 << LED_DDOT_PIN);
    LED_DOT_DDR |= LED_DOT_ALL;
}

/* prepare timers for dynamic indication and internal counting */
void setup_timers(uint8_t *ratio) {
    /* timer for dynamic indication */
    TCNT0 = 0; /* start from zero */
    TCCR0 = (1 << CS01) | (1 << CS00); /* prescaler is 64 */

    /* timer for internal counting */
    OCR1A = 3124; /* count for 0.1 second (ratio = 10 tics / second) */
    *ratio = 10;
    /* some examples
     *
     * count for 1 second (1 tic / second)
     * OCR1A = 31249;
     * *ratio = 1;
     *
     * count for 0.5 second (2 tics / second)
     * OCR1A = 15624;
     * *ratio = 2;
    */
    TCCR1B = (1 << CS12) | (1 << WGM12); /* prescaler is 256 and use CTC mode */

    /* enable overflow interrupt for Timer0 and CTC-match interrupt for Timer1 */
    TIMSK = (1 << TOIE0) | (1 << OCIE1A);
}

/* main program */
int main(void) {
    uint8_t tps_ratio; /* tics per second ratio */
    uint8_t display_mode = SYS_MODE_START; /* what to show on LED screen */
    const unsigned char display_mode_dots[4] = {(1 << LED_DOT_D1), (1 << LED_DOT_D2), (1 << LED_DOT_D3), 0}; /* left-side dots to indicate current display mode */
    unsigned char err_flag = BME280_ERR_NO; /* for tracking errors during measurement cycle */
    unsigned char err_msg[LED_FLD_LEN]; /* error message to be displayed */

    /* prepare I/O lines and check them */
    setup_io();
    LED_test();

    /* configure system timers and set up I2C interface */
    setup_timers(&tps_ratio);
    TWI_setup();

    /* enable interrupts globally */
    sei();

    uint16_t curr_tics;
    uint16_t lastsec = 0;
    uint16_t lasttic = 0;

    while (1) {
        /* save new tics count */
        ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
            curr_tics = SYS_tics;
        }

        int16_t T; /* temperature */
        uint32_t P, H; /* pressure and humidity */
        unsigned char T_alarm, P_alarm, H_alarm; /* alarm flags */

        /* do measurements */
        if (((curr_tics % SYS_MEAS_DELAY) == 0) && (lasttic != curr_tics)) { /* this allows to make measurements exactly one time, at the very first timer tic in new measurement period  */
            lasttic = curr_tics;

            /* TODO reinit connection */
            /*if (err_flag == BME280_ERR_CONN) {
            }*/

            err_flag = BME280_measure(&T, &P, &H, &T_alarm, &P_alarm, &H_alarm);

            if (display_mode == SYS_MODE_START) /* reset start mode */
                display_mode = SYS_MIN_MODE;
        }

        /* process errors during readout */
        switch (err_flag) {
            case BME280_ERR_NO:
                break;

            case BME280_ERR_CONN:
                err_msg[0] = LED_symb_letter_c;
                err_msg[1] = LED_symb_letter_o;
                err_msg[2] = LED_symb_letter_n;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            case BME280_ERR_ID:
                err_msg[0] = LED_symb_letter_i;
                err_msg[1] = LED_symb_letter_d;
                err_msg[2] = LED_symb_empty;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            case BME280_ERR_CALIB:
                err_msg[0] = LED_symb_letter_c;
                err_msg[1] = LED_symb_letter_a;
                err_msg[2] = LED_symb_letter_l;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            case BME280_ERR_ACQ:
                err_msg[0] = LED_symb_letter_a;
                err_msg[1] = LED_symb_letter_c;
                err_msg[2] = LED_symb_letter_q;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            case BME280_ERR_STATUS:
                err_msg[0] = LED_symb_letter_s;
                err_msg[1] = LED_symb_letter_t;
                err_msg[2] = LED_symb_letter_a;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            case BME280_ERR_IO:
                err_msg[0] = LED_symb_letter_i;
                err_msg[1] = LED_symb_letter_o;
                err_msg[2] = LED_symb_empty;
                err_msg[3] = LED_symb_letter_e;
                _delay_ms(1000);

                break;

            default:
                err_msg[0] = LED_symb_letter_e;
                err_msg[1] = err_msg[2] = LED_symb_letter_r;
                err_msg[3] = LED_symb_empty;
                _delay_ms(500);

                break;
        }

        /* if there is no error during readout and T, P or H are out of safety range then blink corresponding LED every 0.5 seconds (hence multiplying by 2) */
        if ((((display_mode == SYS_MODE_T) && (T_alarm)) || ((display_mode == SYS_MODE_P) && (P_alarm)) || ((display_mode == SYS_MODE_H) && (H_alarm))) && (err_flag == BME280_ERR_NO)) {
            if (((curr_tics * 2) / tps_ratio) % 2)
                LED_DOT_PORT &= ~display_mode_dots[display_mode];
            else
                LED_DOT_PORT |= display_mode_dots[display_mode];
        }
        else if (display_mode != SYS_MODE_START) /* otherwise just indicate current mode */
            LED_DOT_PORT |= display_mode_dots[display_mode];

        /* show data */
        switch (display_mode) {
            case SYS_MODE_T:
                if (err_flag)
                    LED_str(err_msg);
                else
                    LED_print_T(T);

                break;

            case SYS_MODE_P:
                if (err_flag)
                    LED_str(err_msg);
                else
                    LED_print_P(P);

                break;

            case SYS_MODE_H:
                if (err_flag)
                    LED_str(err_msg);
                else
                    LED_print_H(H);

                break;

            case SYS_MODE_TIME:
                if ((curr_tics / tps_ratio) % 2) /* blink time dots every second */
                    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);
                else
                    LED_DOT_PORT |= (1 << LED_DOT_D4_5);

                /* TODO check for errors from DS3231 */
                LED_print_time(SYS_tics / tps_ratio); /* actually just a placeholder; real time will be read from DS3231 chip */

                break;

            default:
                LED_dashes(); /* TODO print error */

                break;
        }

        /* iterate over modes */
        if ((((curr_tics / tps_ratio) % SYS_MODE_DELAY) == 0) && (display_mode != SYS_MODE_START)) {
            if (lastsec != (curr_tics / tps_ratio)) {
                lastsec = curr_tics / tps_ratio;

                if (display_mode == SYS_MAX_MODE)
                    display_mode = SYS_MIN_MODE;
                else
                    display_mode++;

                LED_DOT_PORT &= ~LED_DOT_MODES; /* shutdown all mode dots */
            }
        }
    }

    cli();
}

I added delays in switch statement just to be able to see error message. Considering example above (with making BME280_read_id static) on the first iteration I get calE which refers to the problem with downloading calibration coefficients from sensor and on subsequent iterations I get conE, which is related to the problem with reading ID from sensor

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 09:02 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
and the exact text of the error message.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Program compiles, links and flashes just fine.

The only message that could be read is displayed on LED and for the first while loop iteration I get calE and for subsequent conE, if I declare BME280_read_id as static. Or what is the error do you want to see?

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Now I am confused. Above you said:

 However, if I mark with static not a variable but a function (for example, BME280_read_id), then I get error in my main loop (specifically, the first error is BME280_ERR_CAL and then BME280_ERR_CONN).

I simply want to see the exact text of the error you  see in that circumstance.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry for my inexact words. By error I meant error during runtime which is indicated on my led screen. In all three cases program compiles just fine, the only difference is working/non-working cycle

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 10:11 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dviktor wrote:
By error I meant error during runtime which is indicated on my led screen. In all three cases program compiles just fine, the only difference is working/non-working cycle

 

You are basically describing "magical" behavior. There's no way to say what is going on without seeing more information.

 

The basic rule is: All file-scope variables in your program have to be declared as static, unless you want these variables to be global (i.e. linkable from other translation units). And exactly the same applies to functions. Basically, everything that is not mentioned in the module's header file has to be declared static in that module's implementation file. Following this rule will immensely help the compiler to generate the most optimal code.

 

Given your original listing, every function and file-scope variable in it has to be declared static, except for your interface BME280_measure function.

 

As to why your code suddenly stops working... Apparently there's some other reason, not directly related to static at all.

Dessine-moi un mouton

Last Edited: Thu. Aug 29, 2019 - 05:46 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:

If you want to limit the scope of a function, just limit where you #include the header file that declares it.

 

That doesn't really limit anything. As far as the compiler is concerned, a function with external linkage can be called from anywhere by anyone who'd bother to manually spell-out the declaration of that function in their code. The compiler has to assume this "worst case scenario", and this severely limits its code optimization opportunities.

 

Which is why in properly written code everything that can be given internal linkage, has to be given internal linkage. Don't pollute the externally-linkable namespace with unnecessary symbols. This is not just a matter of compilation speed or name conflict avoidance, it is also a matter of helping the compiler generate the most optimal code.

Dessine-moi un mouton

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just look at asm listings. When BME280_read_id is declared as static then there is no .global definition of BME280_read_id inside asm code. There is no signs of BME280_read_id at all!

My error checking is based upon testing return values of the corresponding functions. As you can see every BME280_* routine returns 0 on failure and 1 on success (primarily because of TWI workflow).

 

The first error which occurs when BME280_read_id declared as static is calE which means that BME280_read_calibration call was unsuccessful (see implementation of BME280_measure function). BME280_read_calibration by itself doesn't use any ID-related info. And things work fine if BME280_read_id isn't declared as static. This makes me think that the problem is related to the compilation/optimization issues and placing routines in final binaries in other ways.

 

Should say that without static BME280_read_id function compiled size is:

avr-size -C --mcu=atmega8 main.elf
AVR Memory Usage
----------------
Device: atmega8

Program:    5344 bytes (65.2% Full)
(.text + .data + .bootloader)

Data:         72 bytes (7.0% Full)
(.data + .bss + .noinit)

while with static routine code size is smaller by 24 bytes:

avr-size -C --mcu=atmega8 main.elf
AVR Memory Usage
----------------
Device: atmega8

Program:    5320 bytes (64.9% Full)
(.text + .data + .bootloader)

Data:         72 bytes (7.0% Full)
(.data + .bss + .noinit)

 

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 06:56 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

dviktor wrote:

Just look at asm listings. When BME280_read_id is declared as static then there is no .global definition of BME280_read_id inside asm code. There is no signs of BME280_read_id at all!

 

That's exactly as it should be. All static functions are handled internally by the compiler within a single object file. They do not generate any external (aka "global") symbols and they are completely invisible to the linker.

 

The fact that you don't see a symbol for BME280_read_id does not mean that it got inlined, BTW. It simply means that it produced no global symbol. No more, no less.

 

dviktor wrote:

The first error which occurs when BME280_read_id declared as static is calE which means that BME280_read_calibration call was unsuccessful (see implementation of BME280_measure function). BME280_read_calibration by itself doesn't use any ID-related info. And things work fine if BME280_read_id isn't declared as static. This makes me think that the problem is related to the compilation/optimization issues and placing routines in final binaries in other ways.

 

Just for the sake of experiment, can you try compiling with -fno-inline?

Dessine-moi un mouton

Last Edited: Thu. Aug 29, 2019 - 08:30 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just tried to compile with -fno-inline flag and firmware with static-declared BME280_read_id worked properly! Here are asm listings for nonstatic, static and static with -fno-inline flag versions. Now I'm able to find BME280_read_id symbol in listing, however, without .global flag (as expected).

 

Summary about code size.

non-static version:

avr-size -C --mcu=atmega8 main.elf
AVR Memory Usage
----------------
Device: atmega8

Program:    5344 bytes (65.2% Full)
(.text + .data + .bootloader)

Data:         72 bytes (7.0% Full)
(.data + .bss + .noinit)

static version:

avr-size -C --mcu=atmega8 main.elf
AVR Memory Usage
----------------
Device: atmega8

Program:    5320 bytes (64.9% Full)
(.text + .data + .bootloader)

Data:         72 bytes (7.0% Full)
(.data + .bss + .noinit)

static with -fno-inline compiler flag:

avr-size -C --mcu=atmega8 main.elf
AVR Memory Usage
----------------
Device: atmega8

Program:    5360 bytes (65.4% Full)
(.text + .data + .bootloader)

Data:         72 bytes (7.0% Full)
(.data + .bss + .noinit)

As can be seen latest variant is 16 bytes heavier than non-static version. What is the cause of such problem? What is the right way to use static declarations and what other compiler flags could be tweaked with respect to this?

 

EDIT: I thought that -ffunction-sections and -fdata-sections with -Wl,--gc-sections options may interfere, but here is printout of what have removed by linker:

/usr/bin/avr-ld: removing unused section '.avr.prop' in file 'main.o'
/usr/bin/avr-ld: removing unused section '.text.BME280_reset' in file 'bme280.o'
/usr/bin/avr-ld: removing unused section '.text.LED_clear' in file 'led.o'
/usr/bin/avr-ld: removing unused section '.text.LED_int' in file 'led.o'

Seems like these options are not the cause of the problem

Attachment(s): 

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 09:54 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

If rearranging of the code (due for example to compiler  inlining the read_id function when it's static) changes the behaviour, almost certainly it is a bug lurking in the code somewhere.

It could be timing related, for example in the static case, there is a shorter time from the twi_stop (from the end of inlined read_id) to calling the next function read_calibration.

I don't know if that would cause the problem, but it's just one example of how a small timing change could make a difference.

Small timing changes can also make a difference if you have some race condition with an interrupt etc.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You gave me an idea and I just checked it. Perhaps my TWI_stop() function was wrong:

void TWI_stop(void) {
    TWCR = (1 << TWINT) | (1 << TWSTO) | (1 << TWEN);
}

I haven't waited for finish of issuing of STOP condition. I've modified it:

void TWI_stop(void) {
    TWCR = (1 << TWINT) | (1 << TWSTO) | (1 << TWEN);

    while (TWCR & (1 << TWSTO));
}

and now all works properly with all entries in bme280.c declared as static (with the exception of BME280_measure) and without -fno-inline flag. ELF size is reduced greatly too.

Thank you for the pointing that out!

 

EDIT: however, ld doesn't tell me now that BME280_reset has been excluded. This is strange because I don't use it anywhere...

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU

Last Edited: Thu. Aug 29, 2019 - 10:46 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Very good catch there! The TWI stop might take a couple of cycles, and so might a function return...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dviktor wrote:
EDIT: however, ld doesn't tell me now that BME280_reset has been excluded. This is strange because I don't use it anywhere...

An unused static function would be removed by the compiler, so nothing to do with linking.

In fact if you are using -Wall you should get a warning like

 

main.c:134:13: warning: ‘myg’ defined but not used [-Wunused-function]
 static void myg(void)

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oh, thank you, I got it! Warning is here, you're right

Viktor Drobot
A.N. Belozersky Research Institute Of Physico-Chemical Biology MSU