Dealing with quite long ISR

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

Hello again

 

It's known that ISRs should be as short as possible but sometimes it's difficult to make them so. I'm not very experienced with embedded C-programming but have been using C language for about 10 years and now it's quite difficult for me to change my mind a bit.

 

I'm developing desktop clock with mini meteostation. I use CC25-12 7-segment display to show my info. I use dynamic indication - my ATmega8 switches common cathodes of 7-seg display on and off fast enough with the help of ISR while anodes are set to draw needed symbol. Things works just fine but I have some doubts about code implementation.

 

Actually I have two timers in my system. F_CPU is 8 MHz and Timer0 counts for 255 with prescaler 64 so I'm able to get my TIMER0_OVF interrupt approx every 2ms which is used for dynamic indication. It works pretty good and I don't see any flickering. My second Timer1 count for tics with specified length (e. g. 1 tic equals to 0.1 second and so on). These tics are used in software to switch between display modes - temperature [T], pressure [P], humidity [H] and time (every 5 seconds so 50 tics), display alarm situation if measured params are outside specs (status LED blinks every 0.5 secs so 5 tics), blink two middle clock LEDs (every second so 10 tics) and make measurements of [T], [P] and [H] (every 0.1 secs so 1 tics). Measurements are done with the help of Bosch's BMP280/BME280 via I2C protocol and handled inside eternal loop in main() function. Dynamic indication as well as switching between modes and printing necessary value on 7-seg display is done inside TIMER0_OVF ISR and because of this it become somewhat long (and with calls to functions).

 

And this is the point which confuses me. I feel that I make something wrong. Source code of main.c is attached below (I could upload other modules if there is a need). As for now I use more simple BMP280 so humidity is not shown (only dashes). Also I haven't solved the problem of attaching DS1307 RTC module to my 3.3 VCC setup so instead of real time I print minutes and seconds elapsed from powering on.

 

Because of I little-experienced embedded programmer I would like to get some advices with ISR/main task separation to improve my skills. Any help is greatly appreciated.

 

Source of main.c:

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

#include "bmp280.h"
#include "led_display.h"
#include "twi.h"

#define SYS_MEAS_DELAY  1 // in tics
#define SYS_MODE_DELAY  5 // in seconds

#define SYS_MIN_FLD     0
#define SYS_MAX_FLD     3
#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 const uint8_t mode_dots[4] = {(1 << LED_DOT_D1), (1 << LED_DOT_D2), (1 << LED_DOT_D3), 0}; // modes indicator
volatile const unsigned char fields[4] = {(1 << LED_FLD_1), (1 << LED_FLD_2), (1 << LED_FLD_3), (1 << LED_FLD_4)}; // accessible fields
volatile unsigned char symbols[4]; // corresponding symbols

volatile uint8_t curr_mode = SYS_MODE_START; // current mode
volatile uint8_t curr_fld; // current field

volatile uint32_t tics; // TODO not need such wide integer
volatile uint8_t ratio = 10; // tics to second ratio

int32_t T_print;
uint32_t P_print;
volatile unsigned char T_alarm, P_alarm;

void display_dashes(void) {
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    symbols[0] = LED_symb_dash;
    symbols[1] = LED_symb_dash;
    symbols[2] = LED_symb_dash;
    symbols[3] = LED_symb_dash;
}

void display_print_int(uint32_t i, uint8_t radix) {
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    if ((radix > 16) || (radix == 0))
        display_dashes();

    symbols[0] = LED_symb_digits[i / radix / radix / radix];
    symbols[1] = LED_symb_digits[(i / radix / radix) % radix];
    symbols[2] = LED_symb_digits[(i / radix) % radix];
    symbols[3] = LED_symb_digits[i % radix];
}

void display_print_T(void) {
    LED_DDOT_PORT |= (1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    int32_t T_tmp = T_print;

    if (T_print >= 0)
        symbols[0] = LED_symb_empty;
    else {
        T_tmp = -T_tmp;
        symbols[0] = LED_symb_dash;
    }

    uint32_t Ti, Tf;
    Ti = T_tmp / 100;
    Tf = (T_tmp % 100) / 10;

    if (Ti < 10) {
        symbols[1] = LED_symb_empty;
        symbols[2] = LED_symb_digits[Ti];
    }
    else {
        symbols[1] = LED_symb_digits[Ti / 10];
        symbols[2] = LED_symb_digits[Ti % 10];
    }

    symbols[3] = LED_symb_digits[Tf];
}

void display_print_P(void) {
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    uint32_t Pi, Pf;
    Pi = (P_print * 760) / 101325;
    Pf = (((P_print * 760) % 101325) * 10) / 101325;

    if (Pi > 999) {
        LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
        symbols[0] = LED_symb_digits[Pi / 1000];
        symbols[1] = LED_symb_digits[(Pi / 100) % 10];
        symbols[2] = LED_symb_digits[(Pi / 10) % 10];
        symbols[3] = LED_symb_digits[Pi % 10];
    }
    else {
        LED_DDOT_PORT |= (1 << LED_DDOT_PIN);
        symbols[0] = LED_symb_digits[Pi / 100];
        symbols[1] = LED_symb_digits[(Pi / 10) % 10];
        symbols[2] = LED_symb_digits[Pi % 10];
        symbols[3] = LED_symb_digits[Pf];
    }
}

void display_print_time(void) { // TODO print time from DS1307
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);

    symbols[0] = LED_symb_digits[(((tics / ratio) / 60) / 10) % 10];
    symbols[1] = LED_symb_digits[((tics / ratio) / 60) % 10];
    symbols[2] = LED_symb_digits[(((tics / ratio) % 60) / 10) % 10];
    symbols[3] = LED_symb_digits[((tics / ratio) % 60) % 10];
}

ISR(TIMER0_OVF_vect) {
    LED_SEG_PORT &= ~LED_SEG_ALL; // shut down segments
    LED_SEG_PORT |= symbols[curr_fld];
    LED_FLD_PORT |= LED_FLD_ALL; // pull all fields to high logic (cut-off current)
    LED_FLD_PORT &= ~fields[curr_fld];

    if (curr_fld == SYS_MAX_FLD)
        curr_fld = SYS_MIN_FLD;
    else
        curr_fld++;

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

    if (curr_mode == SYS_MODE_START) {
        display_dashes();
        return;
    }

    if (((curr_mode == SYS_MODE_T) && (T_alarm)) || ((curr_mode == SYS_MODE_P) && (P_alarm))) {
        if (((tics * 2) / ratio) % 2)
            LED_DOT_PORT &= ~mode_dots[curr_mode];
        else
            LED_DOT_PORT |= mode_dots[curr_mode];
    }
    else
        LED_DOT_PORT |= mode_dots[curr_mode];

    switch (curr_mode) {
        case SYS_MODE_T:
            display_print_T();
            break;

        case SYS_MODE_P:
            display_print_P();
            break;

        case SYS_MODE_TIME:
            if ((tics / ratio) % 2)
                LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);
            else
                LED_DOT_PORT |= (1 << LED_DOT_D4_5);

            display_print_time();
            break;

        default:
            display_dashes();
            break;
    }
}

ISR(TIMER1_COMPA_vect) {
    tics++;
}

void setup_io(void) {
    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;
}

void setup_timers(void) {
    TCNT0 = 0; // start from zero
    TCCR0 = (1 << CS01) | (1 << CS00); // prescaler is 64

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

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

void get_tp(void) {
    int32_t T;
    uint32_t P;
    unsigned char aT, aP;

    BMP280_compensate(&T, &P, &aT, &aP);

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        T_print = T;
        T_alarm = aT;
    }

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        P_print = P;
        P_alarm = aP;
        }
}

int main(void) {
    setup_io();
    LED_test();
    setup_timers();
    TWI_setup();

    /* enable interrupts */
    sei();

    uint32_t lastsec_mode = 0; // TODO the same as tics' TODO
    uint32_t lasttic_meas = 0;

    while (1) {
        ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
            if ((((tics / ratio) % SYS_MODE_DELAY) == 0) && (curr_mode != SYS_MODE_START)) {
                if (lastsec_mode != (tics / ratio)) {
                    lastsec_mode = tics / ratio;

                    if (curr_mode == SYS_MAX_MODE)
                        curr_mode = SYS_MIN_MODE;
                    else
                        curr_mode++;
                }
            }
        }

        if ((tics % SYS_MEAS_DELAY) == 0) {
            if (lasttic_meas != tics) {
                lasttic_meas = tics;

                unsigned char id = 0;

                if (!BMP280_read_id(&id)) {
                    display_dashes();
                    continue;
                }

                if (id != BMP280_ID) {
                    display_dashes();
                    continue;
                }

                if (!BMP280_read_calibration()) {
                    display_dashes();
                    continue;
                }

                if (!BMP280_set_acquisition(0x01, 0x01, 0x01)) {
                    display_dashes();
                    continue;
                }

                unsigned char status;

                do {
                    if (!BMP280_read_status(&status)) {
                        LED_DOT_PORT |= (1 << LED_DOT_D1);
                        LED_DOT_PORT &= ~(1 << LED_DOT_D2);

                        display_dashes();

                        status = 1;
                        break;
                    }
                } while (status);

                if (status == 0) {
                    if (!BMP280_read_TP()) {
                        display_dashes();
                        continue;
                    }

                    get_tp();

                    if (curr_mode == SYS_MODE_START)
                        curr_mode = SYS_MIN_MODE;
                }
            }
        }
    }

    cli();
}

 

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

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

I don't see any reason why you could not move your ISR() code to main and poll the overflow flag to update your displays.

 

some thing like in your main loop:

 

If (T0_overflow_flag is set) call T0_ovf(); // rename current isr to T0_ovf()

clear T0_overflow_flag;

.

.

.

 

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"

 

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

With any sort of optimisation, first you need to get some numbers. How long does the isr take to execute? Is this time going to negatively impact your application?

Some comments- generally i’d leave any display logic out of the isr. The isr only takes care of updating the display.
You really only need one timer. You can count interrupts and update ticks at an interval that is a multiple of the timer time. You can use the timer’s CTC mode to get a ‘friendlier’ time period rather than being constrained to the overflow time. For more ideas, i wrote a tutorial on multi-tasking in the tutorials section.

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

ki0bk,

I think that I can leave only that part of TIMER0_OVF ISR:

ISR(TIMER0_OVF_vect) {
    LED_SEG_PORT &= ~LED_SEG_ALL; // shut down segments
    LED_SEG_PORT |= symbols[curr_fld];
    LED_FLD_PORT |= LED_FLD_ALL; // pull all fields to high logic (cut-off current)
    LED_FLD_PORT &= ~fields[curr_fld];

    if (curr_fld == SYS_MAX_FLD)
        curr_fld = SYS_MIN_FLD;
    else
        curr_fld++;

    update_flag = 1;
}

and move the rest inside display_update() function. Will be that good enough? If I get it, in that case ISR will be responsible only for cycling between fields (so-called digits) and lighting on necessary segments... Still not sure whether I should use ATOMIC_BLOCK parts in the rest of my code...

 

Kartman,

I don't know the exact timing for my ISR. I don't think it will mess up I2C communications because of I2C nature - I make it in steps and no actions is taken until TWINT flag is cleared (polling-based TWI seems clearer to me than interrupt-based). I thought about single timer but it seems that for display update I need as fast rate as possible (so Timer0 without CTC) while counting tics for actions which takes time about seconds (or decimal parts of seconds) it's simpler to use Timer1 in CTC mode and set necessary ratio by hand. I may be wrong, I still not feel Timers firmly. Thank you for pointing out your tutorial, I'll have a look.

 

And as I said above, I still don't understand fully the need of ATOMIC_BLOCK wrapper. As for my code I put it in places which is looked critical to me

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

Last Edited: Mon. Dec 17, 2018 - 05:27 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Don't implement the code THEN try to design it into shape!
.
Clear, easily testable and maintainable code results from clear design FIRST.
.
I'd wind back now that you have a working prototype and consider "if I had my time again how would I have actually designed this?". Then do exactly that!

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

But I designed my project on the paper first. The only thing that still puzzles me is the use of ISR and similar staff (never experienced it earlier)

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

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

Main question: Will a long ISR cause problems with your system?  If not, it's not a problem.

 

If it will cause problems, you need to put as much of the ISR code as possible into main (background) code, and just have your ISR code do the most time-critical stuff, and then set a flag for the rest of the code in the background.  Or, you could enable interrupts in the ISR after the most critical code has run, and let the rest of the ISR code be interruptible, if the problem is that other interrupts are blocked for too long.

 

Another thing to do is pre-massage data for the ISR.  Writing out the next digit of a 7-seg display is itself a very fast process.  Have your background code break down the display data into a simple array that holds indexes into a segment table array, so the ISR code merely has to access the segment table array and output the data (along with the correct digit enable line).

Last Edited: Mon. Dec 17, 2018 - 06:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you all for responds! Now I see points where I could make some improvements.

 

kk6gm,

Aren't ISRs are uninterruptable by nature, are they? Perhaps I'm missing something important...

 

One more question (just to not start new topic). I've seen a lot of poling-based TWI implementations (and used one in my program), but there are few ISR-based TWI handling. What is the cause? How you deals with TWI proto?

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

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

dviktor wrote:
What is the cause? How you deals with TWI proto?

As a master, I2C is simple enough not to need ISR() in most cases, it will depend on the complexity of your app.

For slave I2C, it makes sense to use ISR() as you don't know when the master will ask for or present info and need your attention.

 

dviktor wrote:
Aren't ISRs are uninterruptable by nature

Yes, unless you enable interrupts within your ISR(), an advanced topic not suitable for small ram micros like AVR, that most will not need if you keep your ISR()'s short and sweet.

 

 

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"

 

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

dviktor wrote:

kk6gm,

Aren't ISRs are uninterruptable by nature, are they? Perhaps I'm missing something important...

On some families, responding to an interrupt does disable further interrupts.  On other families only interrupts at equal or lower priority are disabled.  You could say both are true of traditional AVRs, since they only have one level of interrupt "priority" (this is different from the hardware priority of response to simultaneous interrupts).

 

There is nothing to stop you from enabling interrupts inside an AVR ISR, but you are then inviting every other ISR in your system to run inside that ISR.  As a general rule you should avoid doing this on an AVR, but that is not to say it must never be done.  That is something only the developer can determine.  But your first approach should always be to make your ISRs as small and fast as possible, to prevent other interrupts from being unduly delayed.  That often means doing the very minimum in the ISR, and doing the rest of the work in main code, as triggered by a flag or command code set in the ISR.  Something like this:

 

bool Int_X_flag = false;

ISR(X)
{
    // do critical code here
    Int_X_flag = true;  // tell background code to finish the job
}

// main loop
while (1)
{
    // other background code
    if (Int_X_flag)
    {
        // do rest of Int_X code here
        Int_X_flag = false; // clear flag for next time thru
    }
}

 

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

I don't have a full picture of the situation.  A 4-digit 7-seg LED?   Shouldn't be much strain, even with bits scattered among ports somewhat.

 

Slight digression:

dviktor wrote:
if (Pi > 999)
How often will that happen? ;)

 

In my 7-seg work (LCD as well as LED but refresh timing requirements are different)  I make a buffer with the bits to be applied to the segments.  Then the refresh part every few milliseconds (I often use ~2.5ms) is then nothing more than turning off all the columns, apply the rows, and turn on the correct column.  As shown in another recent thread, applying a register bit to an I/O bit is a fraction of a microsecond even if the rows are scattered on AVR ports at your clock speed, and less than a microsecond for all rows if on a single port.

 

So, if we say 20us every 2ms, that is 1% of the AVR.  Not very scary.

 

IMO/IME that is the key here -- separating the refresh from the changing of the content.  That [almost] always only needs to be done a few times per second.  If it does happen to fall behind, no big deal.  No need to do that in the "tick" ISR.  Do all the timekeeping and display update in main.

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Yes, this is Kingbright CC25-12EWA LED 7-segment display.

check for 999 is added only for the whole picture :)

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

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

Thank you all for suggestions! I've cleaned my code and reorganized it a bit (saved 616 bytes of flash)

 

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

#include "bmp280.h"
#include "led.h"
#include "twi.h"

#define SYS_MEAS_DELAY  1 // in tics
#define SYS_MODE_DELAY  5 // 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 tics; // WARNING: it should be noted that tics can overflow quick if Timer1 frequency is too high!
uint8_t ratio = 10; // tics per second ratio

volatile unsigned char symbols[4]; // symbols for positions

const unsigned char mode_dots[4] = {(1 << LED_DOT_D1), (1 << LED_DOT_D2), (1 << LED_DOT_D3), 0}; // modes LEDs
uint8_t mode = SYS_MODE_START; // current mode

int16_t T; // current temperature (raw values = [-4000..8500]
uint32_t P; // current pressure (raw values = [30000..110000]
unsigned char T_alarm, P_alarm; // alarm conditions

void print_empty(void);
void print_dashes(void);
void print_T(void);
void print_P(void);
void print_time(void);
void print_int(const uint32_t i, const uint8_t radix);

/* 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)}; // accessible fields
    static uint8_t fld = LED_MIN_FLD; // current field

    LED_SEG_PORT &= ~LED_SEG_ALL; // shut down all segments
    LED_FLD_PORT |= LED_FLD_ALL; // cut off all fields
    LED_SEG_PORT |= symbols[fld]; // enable..
    LED_FLD_PORT &= ~fields[fld]; // ..needed

    // switch to the next field
    if (fld == LED_MAX_FLD)
        fld = LED_MIN_FLD;
    else
        fld++;
}

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

/* prepare I/O lines */
void setup_io(void) {
    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(void) {
    TCNT0 = 0; // start from zero
    TCCR0 = (1 << CS01) | (1 << CS00); // prescaler is 64

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

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

/* main story starts here... */
int main(void) {
    setup_io();
    LED_test();
    setup_timers();
    TWI_setup();

    // enable interrupts globally
    sei();

    uint16_t curr_tics;
    uint16_t lastsec_mode = 0;
    uint16_t lasttic_meas = 0;

    while (1) {
        ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
            curr_tics = tics;
        }

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

        // if T or P are outside of specs then blink status LED every 0.5 secs
        if (((mode == SYS_MODE_T) && (T_alarm)) || ((mode == SYS_MODE_P) && (P_alarm))) {
            if (((curr_tics * 2) / ratio) % 2)
                LED_DOT_PORT &= ~mode_dots[mode];
            else
                LED_DOT_PORT |= mode_dots[mode];
        }
        else if (mode != SYS_MODE_START)
            LED_DOT_PORT |= mode_dots[mode];

        switch (mode) {
            case SYS_MODE_T:
                print_T();
                break;

            case SYS_MODE_P:
                print_P();
                break;

            case SYS_MODE_TIME:
                if ((curr_tics / ratio) % 2)
                    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);
                else
                    LED_DOT_PORT |= (1 << LED_DOT_D4_5);

                print_time();
                break;

            default:
                print_dashes();
                break;
        }

        if ((((curr_tics / ratio) % SYS_MODE_DELAY) == 0) && (mode != SYS_MODE_START)) {
            if (lastsec_mode != (curr_tics / ratio)) {
                lastsec_mode = curr_tics / ratio;

                if (mode == SYS_MAX_MODE)
                    mode = SYS_MIN_MODE;
                else
                    mode++;
            }
        }

        if ((curr_tics % SYS_MEAS_DELAY) == 0) {
            if (lasttic_meas != curr_tics) {
                lasttic_meas = curr_tics;

                unsigned char id = 0;

                if (!BMP280_read_id(&id)) {
                    print_dashes(); // TODO print comm error
                    continue;
                }

                if (id != BMP280_ID) {
                    print_dashes(); // TODO print ID error
                    continue;
                }

                if (!BMP280_read_calibration()) {
                    print_dashes(); // TODO print read error
                    continue;
                }

                if (!BMP280_set_acquisition(0x01, 0x01, 0x01)) {
                    print_dashes(); // TODO print write error
                    continue;
                }

                unsigned char status;

                do {
                    if (!BMP280_read_status(&status)) {
                        print_dashes(); // TODO print read error

                        status = 1;
                        break;
                    }
                } while (status);

                if (status == 0) {
                    if (!BMP280_read_TP()) {
                        print_dashes(); // TODO print read error
                        continue;
                    }

                    BMP280_compensate(&T, &P, &T_alarm, &P_alarm); // perform conversion

                    if (mode == SYS_MODE_START) // reset START mode
                        mode = SYS_MIN_MODE;
                }
                else {
                    print_dashes(); // TODO print status error
                    continue;
                }
            }
        }
    }

    cli();
}

void print_empty(void) {
    // shut down decimal dot and clock dots
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    symbols[0] = symbols[1] = symbols[2] = symbols[3] = LED_symb_empty;
}

void print_dashes(void) {
    // shut down decimal dot and clock dots
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    symbols[0] = symbols[1] = symbols[2] = symbols[3] = LED_symb_dash;
}

void print_T(void) {
    // decimal dot is always on and clock dots are always off
    LED_DDOT_PORT |= (1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    int16_t T_raw = T;

    if (T >= 0)
        symbols[0] = LED_symb_empty;
    else {
        T_raw = -T_raw;
        symbols[0] = LED_symb_dash;
    }

    uint8_t Ti, Tf;
    Ti = T_raw / 100;
    Tf = (T_raw % 100) / 10;

    if (Ti < 10) {
        symbols[1] = LED_symb_empty;
        symbols[2] = LED_symb_digits[Ti];
    }
    else {
        symbols[1] = LED_symb_digits[Ti / 10];
        symbols[2] = LED_symb_digits[Ti % 10];
    }

    symbols[3] = LED_symb_digits[Tf];
}

void print_P(void) {
    // clock dots are always off
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    uint16_t Pi;
    uint8_t Pf;

    Pi = (P * 760) / 101325;
    Pf = (((P * 760) % 101325) * 10) / 101325;

    if (Pi > 999) {
        LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN); // decimal dot is no needed in that case
        symbols[0] = LED_symb_digits[Pi / 1000];
        symbols[1] = LED_symb_digits[(Pi / 100) % 10];
        symbols[2] = LED_symb_digits[(Pi / 10) % 10];
        symbols[3] = LED_symb_digits[Pi % 10];
    }
    else {
        LED_DDOT_PORT |= (1 << LED_DDOT_PIN); // decimal dot is always on
        symbols[0] = LED_symb_digits[Pi / 100];
        symbols[1] = LED_symb_digits[(Pi / 10) % 10];
        symbols[2] = LED_symb_digits[Pi % 10];
        symbols[3] = LED_symb_digits[Pf];
    }
}

void print_time(void) { // TODO print time from DS1307
    // shut off decimal dot pin
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { // TODO only for testing purposes
        symbols[0] = LED_symb_digits[(((tics / ratio) / 60) / 10) % 10];
        symbols[1] = LED_symb_digits[((tics / ratio) / 60) % 10];
        symbols[2] = LED_symb_digits[(((tics / ratio) % 60) / 10) % 10];
        symbols[3] = LED_symb_digits[((tics / ratio) % 60) % 10];
    }
}

void print_int(const uint32_t i, const uint8_t radix) {
    // shut off decimal dot and clock pins
    LED_DDOT_PORT &= ~(1 << LED_DDOT_PIN);
    LED_DOT_PORT &= ~(1 << LED_DOT_D4_5);

    if ((radix > 16) || (radix == 0)) {
        print_dashes();
        return;
    }

    symbols[0] = LED_symb_digits[i / radix / radix / radix];
    symbols[1] = LED_symb_digits[(i / radix / radix) % radix];
    symbols[2] = LED_symb_digits[(i / radix) % radix];
    symbols[3] = LED_symb_digits[i % radix];
}

May be some further suggestions about ISRs?

 

offtopic:

I've found something strange with mega8 TWI interface. If I send TWI_stop condition and then try to connect with slave again (two lines of code following each other without any other actions) then I get RepeatedStart condition, not the usual Start...

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

Last Edited: Tue. Dec 18, 2018 - 08:08 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dviktor wrote:

I've found something strange with mega8 TWI interface. If I send TWI_stop condition and then try to connect with slave again (two lines of code following each other without any other actions) then I get RepeatedStart condition, not the usual Start...

 

This could be the result of a bug in the documentation leading

to a bug in your TWI library.  The datasheet shows the wrong

settings of the TWSTA bit for STOP and REPEATED START but

they don't line up correctly which may have been confusing:

 

ATmega8 TWI stop / repeated start

 

--Mike

 

EDIT: different interpretation of problem with datasheet

 

Last Edited: Wed. Dec 19, 2018 - 05:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dviktor wrote:
It's known that ISRs should be as short as possible

 

That is an interesting rule of thumb and often useful, but not a hard rule.

 

Consider the ISR as a program thread of control.  It can run for as long or short as is of interest.

 

The presumption that interrupts are not preemptable is also false.

Most processors provide support for multiple interrupt levels.

If it does not, you can always make your own priority encoder/scheduler.

 

Analyzing your application for its temporal footprint can help show how to organize code.

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

Am I right that if I use TWSTA + TWSTO + TWINT set then I'll receive 0x08 code?

Now instead of STOP and then START I use repeated start directly (see code example)

 

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()) // RepeatedStart here instead of Stop, Start
        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()) // RepeatedStart here instead of Stop, Start
        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();

    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;
}
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 setting 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()) // RepeatedStart here instead of Stop, Start
        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;
}
#include <util/twi.h>

#include "twi.h"

void TWI_setup(void) {
    /* final SCL frequency is:
     * F_SCL = F_CPU / (16 + 2 * 4^TWSR * TWBR)
     * (100 kHz in our case) */

    TWSR = 0;
    TWBR = 32;
}

uint8_t TWI_start(void) {
    TWCR = (1 << TWINT) | (1 << TWSTA) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    return ((TWSR & 0xF8) == TW_START);
}

uint8_t TWI_rstart(void) {
    TWCR = (1 << TWINT) | (1 << TWSTA) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    return ((TWSR & 0xF8) == TW_REP_START);
}

uint8_t TWI_slave_write(const uint8_t addr) {
    TWDR = (addr << 1) | TWI_WRITE;
    TWCR = (1 << TWINT) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    return ((TWSR & 0xF8) == TW_MT_SLA_ACK);
}

uint8_t TWI_slave_read(const uint8_t addr) {
    TWDR = (addr << 1) | TWI_READ;
    TWCR = (1 << TWINT) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    return ((TWSR & 0xF8) == TW_MR_SLA_ACK);
}

uint8_t TWI_write_byte(const unsigned char byte) {
    TWDR = byte;
    TWCR = (1 << TWINT) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    return ((TWSR & 0xF8) == TW_MT_DATA_ACK);
}

uint8_t TWI_read_byte(unsigned char *byte) {
    TWCR = (1 << TWINT) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    if ((TWSR & 0xF8) != TW_MR_DATA_NACK)
        return 0;

    *byte = TWDR;
    return 1;
}

uint8_t TWI_read_nbytes(unsigned char *buf, const uint8_t n) {
    for (uint8_t i = 0; i < (n - 1); i++) {
        TWCR = (1 << TWINT) | (1 << TWEA) | (1 << TWEN);

        while (!(TWCR & (1 << TWINT)));

        if ((TWSR & 0xF8) != TW_MR_DATA_ACK)
            return 0;

        *(buf + i) = TWDR;
    }

    TWCR = (1 << TWINT) | (1 << TWEN);

    while (!(TWCR & (1 << TWINT)));

    if ((TWSR & 0xF8) != TW_MR_DATA_NACK)
        return 0;

    *(buf + n - 1) = TWDR;

    return 1;
}

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

 

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

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

dviktor wrote:

Am I right that if I use TWSTA + TWSTO + TWINT set then I'll receive 0x08 code?

 

I can't try this at the moment, but the datasheet implies you will

receive 0x08.

 

dviktor wrote:

Now instead of STOP and then START I use repeated start directly (see code example)

 

Repeated START returns 0x10.  It's preferable over STOP + START

if you need to hold onto the bus and there are other masters in

your project who want to use it.

 

--Mike

 

EDIT: over

 

Last Edited: Wed. Dec 19, 2018 - 10:35 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

So, it's better to use (TWSTA + TWSTO + TWINT) directly instead of RepStart? I wonder why my BMP(E)280 still works with RepStart while in datasheet it's said that all read or write transactions via I2C should be followed with STOP...

I'll try START + STOP and post the results

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

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

No, sorry, the sentence I originally wrote could be read two ways.

 

I meant to say a repeated start is preferable.  Otherwise a second

master could grab the bus if it wins the master arbitration.

 

--Mike

 

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

I have only one master on my bus. But if my BMP(E)280 is working, I think it's okay to use RepeatedStart though Stop is required according to I2C transaction diagrams in their datasheet

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