avr-g++ class compilation with -Os option

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

Hi,

 

I have just started to work on an arduino board (atmega328p) in order to learn embedded programming a little bit. Soon I moved to avr toolchain and run into a strange issue which I can't explain. I have a very simple led blinking program:

 

int main(void)
 {
     DDRB |= (1<<5);
     DDRD |= (1<<3);
     DDRD |= (1<<4);
     DDRD |= (1<<5);

     while(1) {
        PINB |= (1<<5);
        PIND |= (1<<3);
        PIND |= (1<<4);
        PIND |= (1<<5);

        _delay_ms(1000);

        PINB &= ~(1<<5);
        PIND &= ~(1<<3);
        PIND &= ~(1<<4);
        PIND &= ~(1<<5);
     }
 }

This code makes the internal led and 3 other leds blinking.

I compile and upload it in the following way:

 

avr-g++ -DF_CPU=16000000UL -mmcu=atmega328p -Os -Wall -o main.cpp.o -c main.cpp
avr-g++ -mmcu=atmega328p main.cpp.o  -o arduino.elf
avr-objcopy -j .text -j .data -O ihex arduino.elf arduino.hex
avrdude -c arduino -p atmega328p -P /dev/ttyACM0 -U flash:w:arduino.hex:i

All goes well. When I create functions in order to manipulate the DDR and PIN, still all good. But when I encapsulate those into a class, like that:

 

class Led
{
public:
    Led(volatile uint8_t* ddr, volatile uint8_t* pin, uint8_t pinId)
    : ddr(ddr)
    , pin(pin)
    , pinId(pinId)
    {
        *ddr |= (1<<pinId);
    }
    
    void on()
    {
        *pin |= (1<<pinId);
    }
    
    void off()
    {
        *pin &= ~(1<<pinId);
    }
    
private:
    volatile uint8_t* ddr;
    volatile uint8_t* pin;
    uint8_t pinId;
};

int main(void)
{
    Led l1(&DDRB, &PINB, 5);
    Led l2(&DDRD, &PIND, 3);
    Led l3(&DDRD, &PIND, 4);
    Led l4(&DDRD, &PIND, 5);

    while(1) {
         l1.on();
         l2.on();
         l3.on();
         l4.on();
         
        _delay_ms(1000);
        
        l1.off();
        l2.off();
        l3.off();
        l4.off();
    }
}

everything falls apart: one of the led is on all the time and only the internal led is blinking. Everything works fine, when I comment out all but one led: the code works well for a single led. Things change if I play with the configuration (how many leds are working), but the result is unexpected.

 

Later on I figured out, when I change the -Os flag in the compiler option into -O1, everything works fine.

 

Please help me to understand what I miss: is it an issue with the code or with compilation? Is it a known issue, maybe already posted here on the forum (I couldn't find anything maybe because of misunderstanding the issue)

 

Thank you

 

 

 

 

This topic has a solution.
Last Edited: Tue. Feb 16, 2021 - 08:17 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I can't believe this works as you intended.

Please check the operation again with this.

 

tamas81 wrote:

 

int main(void)
 {
     DDRB |= (1<<5);
     DDRD |= (1<<3);
     DDRD |= (1<<4);
     DDRD |= (1<<5);

     while(1) {
        PINB |= (1<<5);
        PIND |= (1<<3);
        PIND |= (1<<4);
        PIND |= (1<<5);

        _delay_ms(1000);

        PINB &= ~(1<<5);
        PIND &= ~(1<<3);
        PIND &= ~(1<<4);
        PIND &= ~(1<<5);
     }
 }

 

 

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

And by the same token:

    Led l1(&DDRB, &PINB, 5);
    Led l2(&DDRD, &PIND, 3);
    Led l3(&DDRD, &PIND, 4);
    Led l4(&DDRD, &PIND, 5);

would presumably need to be:

    Led l1(&DDRB, &PORTB, 5);
    Led l2(&DDRD, &PORTD, 3);
    Led l3(&DDRD, &PORTD, 4);
    Led l4(&DDRD, &PORTD, 5);

The point being:

 

DDR is for direction - set it to 0 for inputs, set it to 1 for outputs

PIN is for input - usually you only read it but later chips have an write function which can cause the output state to toggle

PORT is for output - set it to 0 for off/low and 1 for high/on - however if DDR for that pin is 0 (input) then writing to the port bit enables (1) or disables (0) the internal pull up

 

For the simple case of LEDs you should only need to concern yourself with DDR (1's for output) and then 1/0 in the PORT to switch on/ off.

 

BTW this is a personal thing but code like:

    Led(volatile uint8_t* ddr, volatile uint8_t* pin, uint8_t pinId)
    : ddr(ddr)
    , pin(pin)
    , pinId(pinId)

grates. So you are setting one thing called "ddr" to a different input also called "ddr" and same for pin etc. Not everyone agrees but a common convention is to prefix the class member names with m or m_ so you might have:

class Led
{
public:
    Led(volatile uint8_t* ddr, volatile uint8_t* pin, uint8_t pinId)
    : m_ddr(ddr)
    , m_pin(pin)
    , m_pinId(pinId)
    {
        *m_ddr |= (1<<pinId);
    }

private:
    volatile uint8_t* m_ddr;
    volatile uint8_t* m_pin;
    uint8_t m_pinId;
};

or maybe:

class Led
{
public:
    Led(volatile uint8_t* ddr, volatile uint8_t* pin, uint8_t pinId)
    : mDdr(ddr)
    , mPin(pin)
    , mPinId(pinId)
    {
        *mDdr |= (1<<pinId);
    }

private:
    volatile uint8_t* mDdr;
    volatile uint8_t* mPin;
    uint8_t mPinId;
};

PS of course because it is PORT not PIN that you use for output then none of "pin", "m_pin" or "mPin" make much sense anyway - should be "port", "m_port", "mPort" or something like that.

Last Edited: Mon. Feb 15, 2021 - 01:07 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you for checking. What is your concern regarding the code?

 

I compiled again the same way, that's the full code:

 

#include <avr/io.h>
#include <util/delay.h>

int main(void)
{
    DDRB |= (1<<5);
    DDRD |= (1<<3);
    DDRD |= (1<<4);
    DDRD |= (1<<5);

    while(1) {
       PINB |= (1<<5);
       PIND |= (1<<3);
       PIND |= (1<<4);
       PIND |= (1<<5);

       _delay_ms(1000);

       PINB &= ~(1<<5);
       PIND &= ~(1<<3);
       PIND &= ~(1<<4);
       PIND &= ~(1<<5);
    }
}

 

Worked as expected.,, I swear :-)

By the way, the numbering on PIND (3-4-5) is correct, the leds are inserted on "digital port 3-4-5". Maybe you miss the pull-up settings? I originally had them, but removed as it worked that way (I wanted to keep the example code focused and on a minimum level).

 

Thank you

 

 

 

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

Let me check again.
Isn't this code the behavior you want?

    while(1) {
       PINB = (1<<5);
       PIND = (1<<3);
       PIND = (1<<4);
       PIND = (1<<5);

       _delay_ms(1000);
    }

 

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


 

What is your concern regarding the code?

It's not doing what you think it is doing !!

 

As I say in later AVR there is a special feature on the PIN (input) register that, unlike earlier AVRs, you can write to it as well as read from it. But the write is not a "set state" it is "if the bit is 1 then toggle the current output state". So what you think is "working" is only doing so by a fluke!

 

This is what I would have used:

class Led
{
public:
    Led(volatile uint8_t* ddr, volatile uint8_t* port, uint8_t pinId)
    : mDDR(ddr)
    , mPORT(port)
    , mPinID(pinId)
    {
        *mDDR |= (1 << mPinID);
    }

    void on()
    {
        *mPORT |= (1 << mPinID);
    }

    void off()
    {
        *mPORT &= ~(1 << mPinID);
    }

private:
    volatile uint8_t* mDDR;
    volatile uint8_t* mPORT;
    uint8_t mPinID;
};

int main(void)
{
    Led l1(&DDRB, &PORTB, 5);
    Led l2(&DDRD, &PORTD, 3);
    Led l3(&DDRD, &PORTD, 4);
    Led l4(&DDRD, &PORTD, 5);

    while(1) {
         l1.on();
         l2.on();
         l3.on();
         l4.on();

        _delay_ms(1000);

        l1.off();
        l2.off();
        l3.off();
        l4.off();

        _delay_ms(1000); // this is quite important too!
    }
}

However there is a "trick" you can use with micros like mega328P because if you look at the datasheet:

you may spot a "pattern" here? For B, then C then D the register order goes:

 

addr    : PIN

addr+1: DDR

addr+2: PORT

 

and for many of the old school AVR (with the notable exception of PORTF on the mega128) this relationship holds true. The implication of this is that you only need to know one of these thing to know the other two. So say you only knew DDR then the corresponding PIN would be at the DDR-1 location and the corresponding PORT would be at the DDR+1 location. Using this you can use:

public:
    Led(volatile uint8_t* port, uint8_t pinId)
    : mDDR(port - 1)
    , mPORT(port)
    , mPIN(port - 2)
    , mPinID(pinId)
    {
...
private:
    volatile uint8_t* mDDR;
    volatile uint8_t* mPIN;
    volatile uint8_t* mPORT;
    uint8_t mPinID;

Then at the point of instantiation:

    Led l1(&PORTB, 5);
    Led l2(&PORTD, 3);
    Led l3(&PORTD, 4);
    Led l4(&PORTD, 5);

You can see an example of that in some code I wrote:

 

https://github.com/wrightflyer/s...

 

///////////// PORT A //////////////
DualJoystick joy(0, 1, 64); // on A0/A1
// A2 - unused
Button  butJoy(&PORTA, 3, Button::NO_PULL_UP);
Button  but1(  &PORTA, 4, Button::NO_PULL_UP);
Button  but2(  &PORTA, 5, Button::NO_PULL_UP);
Button  but3(  &PORTA, 6, Button::NO_PULL_UP);
Button  but4(  &PORTA, 7, Button::NO_PULL_UP);

///////////// PORT B //////////////
Encoder enc1(  &PORTB, 0, 1);
Encoder enc2(  &PORTB, 2, 3);
Encoder enc3(  &PORTB, 4, 5); // SPI on 5/6/7 (with SS on 4)
// B6 / B7 - unused

///////////// PORT C //////////////
Encoder enc4(  &PORTC, 0, 1);
Button  but5(  &PORTC, 2, Button::NO_PULL_UP); // 2..5 are the JTAG pins
Button  but6(  &PORTC, 3, Button::NO_PULL_UP);
Button  but7(  &PORTC, 4, Button::NO_PULL_UP);
Button  but8(  &PORTC, 5, Button::NO_PULL_UP);
Encoder enc5(  &PORTC, 6, 7);

///////////// PORT D //////////////
Uart uart(9600); // on D0 / D1
Encoder enc6(  &PORTD, 2, 3);
Encoder enc7(  &PORTD, 4, 5);
Encoder enc8(  &PORTD, 6, 7);

These are principally all "input" devices (buttons use one DDR and PIN, encoders use two DDR and PIN bits) but I only specify PORTx in each case (and the pin number(s)). Then the c'tors do something like:

 

https://github.com/wrightflyer/s...

Button::Button(volatile uint8_t * port, uint8_t pin, uint8_t pullup) : mCount(0) {
    mPORT = port;
    mDDR  = port - 1;
    mPIN  = port - 2;
    mPinMask = (1 << pin);

https://github.com/wrightflyer/s...

Encoder::Encoder(volatile uint8_t * port, uint8_t pin1, uint8_t pin2) :
                mPos(0)
{
    mPin1 = (1 << pin1);
    mPin2 = (1 << pin2);
    mPORT = port;
    mDDR  = port - 1;
    mPIN  = port - 2;
    // this uses the "trick" that GPIO ports in memory are laid out as:
    // n + 0: PINx
    // n + 1: DDRx
    // n + 2: PORTx
    //
    // so given PORTx you can write DDR with (PORT-1) and PIN with (PORT-2)

PS and just for the record here is that mega128 exception for port F:

 

 

It looks like PINF should have been at location 0x60 but turned up at 0x00 instead !

Last Edited: Mon. Feb 15, 2021 - 01:44 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

clawson wrote:
It looks like PINF should have been at location 0x60 but turned up at 0x00 instead !

That was done to stop the wave of "simplifications" in accessing microcontroller pins.  But indeed it is quite important to be able to override in pin work, when the device changes pin connections at run time, I'll give you that.

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: 1

theusch wrote:
when the device changes pin connections at run time, I'll give you that.
wink

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

You can account for differences in register order-

 

https://godbolt.org/z/98b3aE

ver 3.0, now has pin toggle capability information included (non-template version uses, the template version was left as-is)

I think ver 3.0 is the final version.

 

The same place you create the mcu specific pin enums (which will enforce using only available pins), you can create the port register order and how you calculate the specific pin's port base address.

 

Also note that when creating the non-template class version, if you create it in a local scope the compiler will most likely optimize to minimal (as seen in example), but place the creation in a global scope and you will then get a jump in code size as all the pin information now has to have storage on the mcu. When this happens, you no longer get cbi/sbi because they are encoded instructions that have to be done at compile time. Since potentially no cbi/sbi on the register writes (except for PINx, which can be atomic in any case if the mcu has that ability), then you have to interrupt guard any register writes (as this is a pin class, all writes are bit manipulations).

 

edit- I missed adding in the cli in InterruptLock, now added.

Last Edited: Mon. Feb 15, 2021 - 08:28 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you all of your help. What a mistake. The key of course was, that during learning these registers somehow I messed up with the names (PIN <-> PORT). And it didn't help, that it seemed to work. But there was an other very important factor, which you mentioned: that the second delay was missing and the leds never turned off when I used PORT instead of PIN. It is a strange coincidence though, that PIN worked as well (I tested even today, because I couldn't believe, but I confirm, that the code "works" - the frequency is different though).

 

Thank you for replying so patiently for such dumb question, the first steps seem to be more difficult than I expected ;-)