C++ Class

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

Hi Folks,

 

It has been some time since I been around on these forums and recently just started coding again for the AVRs.  I making a small HVAC controller to use at work, it's basically pushbuttons to control functions of a HVAC.  I used some of my old coding from another project which was never put in use.  From what I can recalled, the coding worked great.  I basically trying to make better.  I used CPUSHBUTTON_CONTROL for each LED/Button and another called CPUSHBUTTON_UI in a set, the UI mainly controls the LED/Buttons.  Here's a updated version of the CONTROL Class.

 

#include "CPUSHBUTTON_CONTROL.h"

typedef struct {
	
	volatile uint8_t* PORT;
	volatile uint8_t* DDR;
	volatile uint8_t MASK;
	volatile uint8_t* PIN;
	
} sPORT_INFO;

class CPUSHBUTTON_CONTROL {
	
	private:
	
		volatile sPORT_INFO mSW_PRIMARY_LED;
		volatile sPORT_INFO mSW_SECONDARY_LED;
		volatile sPORT_INFO mSW;
		volatile sPORT_INFO mPOWER_RELAY;
	
		/*volatile uint8_t* mSW_PRIMARY_LED_PORT;
		volatile uint8_t* mSW_PRIMARY_LED_DDR;
		volatile uint8_t mSW_PRIMARY_LED_MASK;
		volatile uint8_t* mSW_PRIMARY_LED_PIN;
		
		volatile uint8_t* mSW_SECONDARY_LED_PORT;
		volatile uint8_t* mSW_SECONDARY_LED_DDR;
		volatile uint8_t mSW_SECONDARY_LED_MASK;
		volatile uint8_t* mSW_SECONDARY_LED_PIN;

		volatile uint8_t* mSW_PORT;
		volatile uint8_t* mSW_DDR;
		volatile uint8_t mSW_MASK;
		volatile uint8_t* mSW_PIN;
		
		volatile uint8_t* mPOWER_RELAY_PORT;
		volatile uint8_t* mPOWER_RELAY_DDR;
		volatile uint8_t mPOWER_RELAY_MASK;
		volatile uint8_t* mPOWER_RELAY_PIN;*/
		
	public:
	
		CPUSHBUTTON_CONTROL();
		
		void SETUP_SW_PRIMARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
		void SETUP_SW_SECONDARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
		void SETUP_SW(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
		void SETUP_POWER_RELAY(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
		
		void INIT();
		int READ_SW(uint8_t auto_reset_function = YES);
	
};

CPUSHBUTTON_CONTROL::CPUSHBUTTON_CONTROL() {

}

void CPUSHBUTTON_CONTROL::SETUP_SW_PRIMARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mSW_PRIMARY_LED.PORT = PORT;
	this->mSW_PRIMARY_LED.DDR = DDR;
	this->mSW_PRIMARY_LED.MASK = MASK;
	this->mSW_PRIMARY_LED.PIN = PIN;
	
	/*this->mSW_PRIMARY_LED_PORT = PORT;
	this->mSW_PRIMARY_LED_DDR = DDR;
	this->mSW_PRIMARY_LED_MASK = MASK;
	this->mSW_PRIMARY_LED_PIN = PIN;*/
}

void CPUSHBUTTON_CONTROL::SETUP_SW_SECONDARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mSW_SECONDARY_LED.PORT = PORT;
	this->mSW_SECONDARY_LED.DDR = DDR;
	this->mSW_SECONDARY_LED.MASK = MASK;
	this->mSW_SECONDARY_LED.PIN = PIN;
	
	/*this->mSW_SECONDARY_LED_PORT = PORT;
	this->mSW_SECONDARY_LED_DDR = DDR;
	this->mSW_SECONDARY_LED_MASK = MASK;
	this->mSW_SECONDARY_LED_PIN = PIN;*/
}

void CPUSHBUTTON_CONTROL::SETUP_SW(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mSW.PORT = PORT;
	this->mSW.DDR = DDR;
	this->mSW.MASK = MASK;
	this->mSW.PIN = PIN;
	
	/*this->mSW_PORT = PORT;
	this->mSW_DDR = DDR;
	this->mSW_MASK = MASK;
	this->mSW_PIN = PIN;*/
}

void CPUSHBUTTON_CONTROL::SETUP_POWER_RELAY(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mPOWER_RELAY.PORT = PORT;
	this->mPOWER_RELAY.DDR = DDR;
	this->mPOWER_RELAY.MASK = MASK;
	this->mPOWER_RELAY.PIN = PIN;
	
	/*this->mPOWER_RELAY_PORT = PORT;
	this->mPOWER_RELAY_DDR = DDR;
	this->mPOWER_RELAY_MASK = MASK;
	this->mPOWER_RELAY_PIN = PIN;*/
}

void CPUSHBUTTON_CONTROL::INIT() {
	//Let Make Sure Our Pullup Is Turn Off
	*this->mSW.PORT &= ~(this->mSW.MASK);
	//Set Our Pushbutton To Input
	*this->mSW.DDR &= ~(this->mSW.MASK);

	//Let Make Sure Our Primary LED is Turn Off
	*this->mSW_PRIMARY_LED.PORT &= ~(this->mSW_PRIMARY_LED.MASK);
	//Set Our Primary LED To Output
	*this->mSW_PRIMARY_LED.DDR |= (this->mSW_PRIMARY_LED.MASK);
	
	//Let Make Sure Our Secondary LED is Turn Off
	*this->mSW_SECONDARY_LED.PORT &= ~(this->mSW_SECONDARY_LED.MASK);
	//Set Our Secondary LED To Output
	*this->mSW_SECONDARY_LED.DDR |= (this->mSW_SECONDARY_LED.MASK);
	
	//Let Make Sure Our Power Relay Is Turn Off
	*this->mPOWER_RELAY.PORT &= ~(this->mPOWER_RELAY.MASK);
	//Set Our Power Relay To Output
	*this->mPOWER_RELAY.DDR |= (this->mPOWER_RELAY.MASK);
}

int CPUSHBUTTON_CONTROL::READ_SW(uint8_t auto_reset_function) {
	
}

As you can see, I updated the PORT info into a struct and this one has dual color LEDs, the old one didn't.  As you can see SW, LEDs, Power Relay has the same struct.  I wanted a way to use one setup function to setup each LEDs, SW, Relays without having four functions to setup each one.  Trying to find ways to reduce the program size.  Anyone has any tips?

 

PS:  What is the different between .hex and .elf files?  I noticed that the .hex is smaller than the .elf.  .elf is becoming big.  Which file are you suppose to use when uploading into a AVR?

Thanks
Shane

Last Edited: Sun. Jan 25, 2015 - 11:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

.elf is the primary output of the compiler/linker. Has lots of debugging info in it.

.hex is your code and constants, essentially a textually coded memory image. But .hex files are formatted as human readable ASCII text per the Intel Hex de facto standard.  Most bootloader PC side software use .hex.

Debugger pods (SWD, JTAG) use .elf.

 

A .hex can be converted to a binary file - that's the bytes of a memory image. A few bootloaders or memory programmers use binary (.bin) files.

 

As to the C++ - at a glance it's much more wordy and has more pointers and variables than needed. It is fundamentally well structutred.

But I'm just not motivated to pore over the code right now!

 

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

It is much easier to read the new code that is using the struct without all of the commented out old code sections.

#include "CPUSHBUTTON_CONTROL.h"

typedef struct {

    volatile uint8_t* PORT;
    volatile uint8_t* DDR;
    volatile uint8_t MASK;
    volatile uint8_t* PIN;

} sPORT_INFO;

class CPUSHBUTTON_CONTROL {

private:

    volatile sPORT_INFO mSW_PRIMARY_LED;
    volatile sPORT_INFO mSW_SECONDARY_LED;
    volatile sPORT_INFO mSW;
    volatile sPORT_INFO mPOWER_RELAY;

public:

    CPUSHBUTTON_CONTROL();

    void SETUP_SW_PRIMARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
    void SETUP_SW_SECONDARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
    void SETUP_SW(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
    void SETUP_POWER_RELAY(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);

    void INIT();
    int READ_SW(uint8_t auto_reset_function = YES);

};

CPUSHBUTTON_CONTROL::CPUSHBUTTON_CONTROL() {

}

void CPUSHBUTTON_CONTROL::SETUP_SW_PRIMARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
    this->mSW_PRIMARY_LED.PORT = PORT;
    this->mSW_PRIMARY_LED.DDR = DDR;
    this->mSW_PRIMARY_LED.MASK = MASK;
    this->mSW_PRIMARY_LED.PIN = PIN;
}

void CPUSHBUTTON_CONTROL::SETUP_SW_SECONDARY_LED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
    this->mSW_SECONDARY_LED.PORT = PORT;
    this->mSW_SECONDARY_LED.DDR = DDR;
    this->mSW_SECONDARY_LED.MASK = MASK;
    this->mSW_SECONDARY_LED.PIN = PIN;
}

void CPUSHBUTTON_CONTROL::SETUP_SW(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
    this->mSW.PORT = PORT;
    this->mSW.DDR = DDR;
    this->mSW.MASK = MASK;
    this->mSW.PIN = PIN;
}

void CPUSHBUTTON_CONTROL::SETUP_POWER_RELAY(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
    this->mPOWER_RELAY.PORT = PORT;
    this->mPOWER_RELAY.DDR = DDR;
    this->mPOWER_RELAY.MASK = MASK;
    this->mPOWER_RELAY.PIN = PIN;
}

void CPUSHBUTTON_CONTROL::INIT() {
    //Let Make Sure Our Pullup Is Turn Off
    *this->mSW.PORT &= ~(this->mSW.MASK);
    //Set Our Pushbutton To Input
    *this->mSW.DDR &= ~(this->mSW.MASK);

    //Let Make Sure Our Primary LED is Turn Off
    *this->mSW_PRIMARY_LED.PORT &= ~(this->mSW_PRIMARY_LED.MASK);
    //Set Our Primary LED To Output
    *this->mSW_PRIMARY_LED.DDR |= (this->mSW_PRIMARY_LED.MASK);

    //Let Make Sure Our Secondary LED is Turn Off
    *this->mSW_SECONDARY_LED.PORT &= ~(this->mSW_SECONDARY_LED.MASK);
    //Set Our Secondary LED To Output
    *this->mSW_SECONDARY_LED.DDR |= (this->mSW_SECONDARY_LED.MASK);

    //Let Make Sure Our Power Relay Is Turn Off
    *this->mPOWER_RELAY.PORT &= ~(this->mPOWER_RELAY.MASK);
    //Set Our Power Relay To Output
    *this->mPOWER_RELAY.DDR |= (this->mPOWER_RELAY.MASK);
}

int CPUSHBUTTON_CONTROL::READ_SW(uint8_t auto_reset_function) {

}

 

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Mon. Jan 26, 2015 - 07:57 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry but why do you have to dereference "this"? It's surely a C++  class? "this" is implied anyway?

class Foo {
public:
    Foo();
    void set(int n);
    int get();
private:
    int mFoovar;
};

Foo myfoo;

void Foo:set(int n) {
    mFoovar = n;
}

int Foo:get() {
    return mFoovar;
}

int main(void) {
    myfoo.set(37);
    PORTB = myfoo.get();
}

Not a sign of "this" anywhere because when the setter and getter access mFoovar it is "this->mFoovar" by implication anyway.

 

Also why not take the "per LED" settings as parameters to the constructir and use member initialisation in that? ie along the lines of:

class Led {
    Led(uint8_t * port, uint8_t * ddr);
private:
    uint8_t * mPort;
    uint8_t * mDdr;
};

Led::Led(uint8_t * port, uint8_t * ddr) : mPort(port), mDdr(ddr) {
    // all done in the initialiation
}

Led red(&PORTB, &DDRB);
Led green(&PORTD, &DDRD);

etc.

I won't mention things like listing public: before private: as I suppose, to a certain extent, that's down to personal style (but surely the reader is only really interested in what you are making public: so you put that first?)

 

OH and there's a "trick" you can use on AVRs that works for almost every port of every AVR. The registers are always in the order:

 

PIN

DDR

PORT

 

So you only need be given one address. For example if you are given DDRB then the PIN register is addr-1 and the PORT register is addr+1 though I guess the user is most likely to want to use:

Led red(&PORTB);

in which case DDR is addr-1 and PIN is addr-2. The point being they don't have to pass all of PORTB, DDRB, PINB.

 

No doubt someone will be along in a minute to point out some example of an AVR/port where this three address sequence is broken! ;-)

Last Edited: Mon. Jan 26, 2015 - 09:42 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
No doubt someone will be along in a minute to point out some example of an AVR/port where this three address sequence is broken! ;-)

ATMega64/128 PORTF.

(three minutes smiley)

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

Hats off to you!

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

snigelen wrote:
ATMega64/128 PORTF.
I wonder why.  PINF could have been at 0x60 which is not used.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

snigelen wrote:
ATMega64/128 PORTF.

That breaks my IOPorts class.  Back to the drawing board.

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Mon. Jan 26, 2015 - 08:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

So are you saying I should create a class for LED, SW, and Power Relay?  Keep in mind that there also functions that controls the led etc which is not in the current coding yet.  Now the this-> is just pointing to the class ain't it?  That what I wanted it to do in case I have some local variables in a function later down the road.  I had always done it like that even in PHP.  Not sure what you mean by dereference.

 

clawson wrote:

Sorry but why do you have to dereference "this"? It's surely a C++  class? "this" is implied anyway?

class Foo {
public:
    Foo();
    void set(int n);
    int get();
private:
    int mFoovar;
};

Foo myfoo;

void Foo:set(int n) {
    mFoovar = n;
}

int Foo:get() {
    return mFoovar;
}

int main(void) {
    myfoo.set(37);
    PORTB = myfoo.get();
}

Not a sign of "this" anywhere because when the setter and getter access mFoovar it is "this->mFoovar" by implication anyway.

 

Also why not take the "per LED" settings as parameters to the constructir and use member initialisation in that? ie along the lines of:

class Led {
    Led(uint8_t * port, uint8_t * ddr);
private:
    uint8_t * mPort;
    uint8_t * mDdr;
};

Led::Led(uint8_t * port, uint8_t * ddr) : mPort(port), mDdr(ddr) {
    // all done in the initialiation
}

Led red(&PORTB, &DDRB);
Led green(&PORTD, &DDRD);

etc.

I won't mention things like listing public: before private: as I suppose, to a certain extent, that's down to personal style (but surely the reader is only really interested in what you are making public: so you put that first?)

 

OH and there's a "trick" you can use on AVRs that works for almost every port of every AVR. The registers are always in the order:

 

PIN

DDR

PORT

 

So you only need be given one address. For example if you are given DDRB then the PIN register is addr-1 and the PORT register is addr+1 though I guess the user is most likely to want to use:

Led red(&PORTB);

in which case DDR is addr-1 and PIN is addr-2. The point being they don't have to pass all of PORTB, DDRB, PINB.

 

No doubt someone will be along in a minute to point out some example of an AVR/port where this three address sequence is broken! ;-)

Thanks
Shane

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

Here's an update on this using a CLED class.

 

class CLED {
	
	public:
	
		CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
		void INIT();
		uint8_t STATUS();
		void LED_OFF();
		void LED_ON();
		void LED_INVERT();
		void FLASH_TIMER(int time[], uint8_t status[], uint8_t array_size, int reset_time = -200);
	
	private:
		volatile sPORT_INFO mLED;
		
};

CLED::CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mLED.PORT = PORT;
	this->mLED.DDR = DDR;
	this->mLED.MASK = MASK;
	this->mLED.PIN = PIN;
	
	this->INIT();
}

void CLED::INIT() {
	//Let Make Sure Our LED is Turn Off
	//*this->mLED.PORT &= ~(this->mLED.MASK);
	this->LED_OFF();
	//Set Our LED To Output
	*this->mLED.DDR |= (this->mLED.MASK);
}

uint8_t CLED::STATUS() {
	if ((*this->mLED.PIN & (this->mLED.MASK)) != 0) {
			return ON;
		} else {
			return OFF;
	}
}

void CLED::LED_OFF() {
	if (this->STATUS() == ON) {
		*this->mLED.PORT &= ~(this->mLED.MASK);
	}
}

void CLED::LED_ON() {
	if (this->STATUS() == OFF) {
		*this->mLED.PORT |= (this->mLED.MASK);
	}
}

void CLED::LED_INVERT() {
	*this->mLED.PORT ^= (this->mLED.MASK);
}

void CLED::FLASH_TIMER(int time[], uint8_t status[], uint8_t array_size, int reset_time) {
	
}

 

Thanks
Shane

Last Edited: Mon. Jan 26, 2015 - 11:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Content moved to new thread.

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Tue. Jan 27, 2015 - 02:37 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Now the this-> is just pointing to the class ain't it?

 No, it points to the local instance of the class. The point is that it is unnecessary since the only thing you have access to is the local instance.

So are you saying I should create a class for LED, SW, and Power Relay?

At the moment you only need one class since as it stands they all have the same functionality. However, later you would descend from that class to get the variations needed for each.

Regards,
Steve A.

The Board helps those that help themselves.

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

My point above and that Steve reiterated was that this:

CLED::CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	this->mLED.PORT = PORT;
	this->mLED.DDR = DDR;
	this->mLED.MASK = MASK;
	this->mLED.PIN = PIN;
	
	this->INIT();
}

(for example) can simply be written as:

CLED::CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) {
	mLED.PORT = PORT;
	mLED.DDR = DDR;
	mLED.MASK = MASK;
	mLED.PIN = PIN;
	
	INIT();
}

and it will work identically. (and be MUCH more readable!).

 

In fact in C++ it's actually very unusual to ever need to refer to "this".

 

In fact that code can actually be written much simpler:

#include  <stdint.h>

#define ON 1
#define OFF 0

class CLED {
	
public:

	CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN);
	void INIT();
	uint8_t STATUS();
	void LED_OFF();
	void LED_ON();
	void LED_INVERT();
	void FLASH_TIMER(int time[], uint8_t status[], uint8_t array_size, int reset_time = -200);

private:
	volatile uint8_t * PORT;
	volatile uint8_t * DDR;
	uint8_t MASK;
	volatile uint8_t * PIN;
};

CLED::CLED(volatile uint8_t* PORT, volatile uint8_t* DDR, volatile uint8_t MASK, volatile uint8_t* PIN) :
	PORT(PORT), DDR(DDR), MASK(MASK), PIN(PIN)
{
	INIT();
}

void CLED::INIT() {
	//Let Make Sure Our LED is Turn Off
	//*this->mLED.PORT &= ~(this->mLED.MASK);
	LED_OFF();
	//Set Our LED To Output
	*DDR |= MASK;
}

uint8_t CLED::STATUS() {
	if ((*PIN & MASK) != 0) {
			return ON;
		} else {
			return OFF;
	}
}

void CLED::LED_OFF() {
	if (STATUS() == ON) {
		*PORT &= ~MASK;
	}
}

void CLED::LED_ON() {
	if (STATUS() == OFF) {
		*PORT |= MASK;
	}
}

void CLED::LED_INVERT() {
	*PORT ^= MASK;
}

void CLED::FLASH_TIMER(int time[], uint8_t status[], uint8_t array_size, int reset_time) {
	
}

int main(void) {
}

In this I simply discarded your sPORT_INFO all together. I don't see the point of grouping PORT/DDR/MASK/PIN separately. A class is, itself, your opportunity to aggregate the items relevant to an LED  - this is, after all, an "LED" class - so why bury this at another level?

 

Also I personally do not agree with anything but #define's being in upper case (the fact that they are upper case is what says they are a macro) so my final version would probably be something like:

#include  <stdint.h>

#define ON 1
#define OFF 0

class Cled {
public:
	Cled(volatile uint8_t* pPort, volatile uint8_t* pDdr, volatile uint8_t mask, volatile uint8_t* pPin);
	void init();
	uint8_t status();
	void LED_off();
	void LED_on();
	void LED_invert();
	void flash_timer(int time[], uint8_t status[], uint8_t array_size, int reset_time = -200);

private:
	volatile uint8_t * mPort;
	volatile uint8_t * mDDr;
	uint8_t mMask;
	volatile uint8_t * mPin;
};

Cled::Cled(volatile uint8_t* pPort, volatile uint8_t* pDdr, volatile uint8_t mask, volatile uint8_t* pPin) :
	mPort(pPort), mDdr(pDdr), mMask(mask), mPin(pPin)
{
	init();
}

void Cled::init() {
	//Let Make Sure Our LED is Turn Off
	LED_off();
	//Set Our LED To Output
	*mDDr |= mMask;
}

uint8_t Cled::status() {
	if ((*mPin & mMask) != 0) {
			return ON;
		} else {
			return OFF;
	}
}

void Cled::LED_off() {
	if (status() == ON) {
		*mPort &= ~mMask;
	}
}

void Cled::LED_on() {
	if (status() == OFF) {
		*mPort |= mMask;
	}
}

void Cled::LED_invert() {
	*mPort ^= mMask;
}

void Cled::flash_timer(int time[], uint8_t status[], uint8_t array_size, int reset_time) {
	
}

int main(void) {
}

I allowed myself the luxury of keeping "LED" in upper case there simply because it's the widely recognised acronym though I didn't use it in the class name (Cled) simply because that would have put the whole thing in upper case and look like a macro name again. Obviously "ON" and "OFF" are in upper case (as I wrote them) because they ARE macro names. There's some argument to say they should probably be "static const uint8_t" but even if they were there might be a temptation to keep them all upper case even if it does break my naming convention ;-)

Last Edited: Tue. Jan 27, 2015 - 11:20 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
void Cled::LED_off() {
	if (status() == ON) {
		*mPort &= ~mMask;
	}
}
void CLED::LED_OFF() {
	if (this->STATUS() == ON) {
		*this->mLED.PORT &= ~(this->mLED.MASK);
	}
}

Just for the sake of discussion, what kind of code does this (pun intended) construct generate, when in a real-enough program so it doesn't get resolved at compile time?  I guess that would be more than one instance of CPUSHBUTTON_CONTROL, using more than one PORT/pin?

 

How many cycles does it take to turn that LED off?  Are there extra RMW considerations, even just considering "low" ports reachable by SBI/CBI? 

 

I seem to recall a GCC forum thread where a "low overhead" or "no overhead" C++ facility for this was presented.

 

Given the AVR8 instruction set, I've never been a fan of "virtual I/O" on an AVR8, C++ or otherwise.  Does your board layout change on the fly?

 

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

Just for the sake of discussion, what kind of code does this (uggh!) construct generate

Lee,

 

Just to say that I was testing this using "gcc" (ie the amd64_x86 host version on my Linux PC) not avr-gcc so I cannot easily see what AVR code this would create. Nothing in what I built was actually AVR specific (note no inclusion of <avr/io.h> there) as I was just "playing" with C++ syntax and that's the same whether you build for AVR, ARM, MIPS, x86 or whatever.

 

When I get a chance I'll build this code on a machine with avr-g++ and see what it generates for a typical:

Cled foo(&PORTB, &DDRB, 0x40, &PINB);

Watch this space...

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

OK so I just added the <avr/io.h> and the following at the end:

Cled Red(&PORTB, &DDRB, (1 << PB5), &PINB);

int main(void) {
	Red.init();
	Red.LED_on();
	Red.LED_off();
	Red.LED_invert();
}

and the annotated source says this:

	.text
.Ltext0:

	.section	.text._ZN4Cled6statusEv,"ax",@progbits
.global	_ZN4Cled6statusEv
	.type	_ZN4Cled6statusEv, @function
_ZN4Cled6statusEv:
//==> uint8_t Cled::status() {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	if ((*mPin & mMask) != 0) {
	movw r26,r24
	adiw r26,5
	ld r30,X+
	ld r31,X
	sbiw r26,5+1
	ld r18,Z
	adiw r26,4
	ld r24,X
	mov r25,r18
	and r25,r24
	ldi r24,lo8(1)
	brne .L2
	ldi r24,0
.L2:
//==> }
	ret
	.size	_ZN4Cled6statusEv, .-_ZN4Cled6statusEv

	.section	.text._ZN4Cled7LED_offEv,"ax",@progbits
.global	_ZN4Cled7LED_offEv
	.type	_ZN4Cled7LED_offEv, @function
_ZN4Cled7LED_offEv:
//==> void Cled::LED_off() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	if (status() == ON) {
	call _ZN4Cled6statusEv
	cpi r24,lo8(1)
	brne .L3
//==> 		*mPort &= ~mMask;
	ld r30,Y
	ldd r31,Y+1
	ld r25,Z
	ldd r24,Y+4
	com r24
	and r24,r25
	st Z,r24
.L3:
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled7LED_offEv, .-_ZN4Cled7LED_offEv

	.section	.text._ZN4Cled4initEv,"ax",@progbits
.global	_ZN4Cled4initEv
	.type	_ZN4Cled4initEv, @function
_ZN4Cled4initEv:
//==> void Cled::init() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	LED_off();
	call _ZN4Cled7LED_offEv
//==> 	*mDdr |= mMask;
	ldd r30,Y+2
	ldd r31,Y+3
	ld r25,Z
	ldd r24,Y+4
	or r24,r25
	st Z,r24
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled4initEv, .-_ZN4Cled4initEv

	.section	.text._ZN4CledC2EPVhS1_hS1_,"ax",@progbits
.global	_ZN4CledC2EPVhS1_hS1_
	.type	_ZN4CledC2EPVhS1_hS1_, @function
_ZN4CledC2EPVhS1_hS1_:
//==> Cled::Cled(volatile uint8_t* pPort, volatile uint8_t* pDdr, volatile uint8_t mask, volatile uint8_t* pPin) :
	push r16
	push r17
	push r28
	push r29
	push __zero_reg__
	in r28,__SP_L__
	in r29,__SP_H__
/* prologue: function */
/* frame size = 1 */
/* stack size = 5 */
.L__stack_usage = 5
	std Y+1,r18
//==> mPort(pPort), mDdr(pDdr), mMask(mask), mPin(pPin)
	movw r30,r24
	std Z+1,r23
	st Z,r22
	std Z+3,r21
	std Z+2,r20
	ldd r18,Y+1
	std Z+4,r18
	std Z+6,r17
	std Z+5,r16
//==> 	init();
	call _ZN4Cled4initEv
/* epilogue start */
//==> }
	pop __tmp_reg__
	pop r29
	pop r28
	pop r17
	pop r16
	ret
	.size	_ZN4CledC2EPVhS1_hS1_, .-_ZN4CledC2EPVhS1_hS1_
.global	_ZN4CledC1EPVhS1_hS1_
	.set	_ZN4CledC1EPVhS1_hS1_,_ZN4CledC2EPVhS1_hS1_

	.section	.text._ZN4Cled6LED_onEv,"ax",@progbits
.global	_ZN4Cled6LED_onEv
	.type	_ZN4Cled6LED_onEv, @function
_ZN4Cled6LED_onEv:
//==> void Cled::LED_on() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	if (status() == OFF) {
	call _ZN4Cled6statusEv
	cpse r24,__zero_reg__
	rjmp .L7
//==> 		*mPort |= mMask;
	ld r30,Y
	ldd r31,Y+1
	ld r25,Z
	ldd r24,Y+4
	or r24,r25
	st Z,r24
.L7:
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled6LED_onEv, .-_ZN4Cled6LED_onEv

	.section	.text._ZN4Cled10LED_invertEv,"ax",@progbits
.global	_ZN4Cled10LED_invertEv
	.type	_ZN4Cled10LED_invertEv, @function
_ZN4Cled10LED_invertEv:
//==> void Cled::LED_invert() {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	*mPort ^= mMask;
	movw r26,r24
	ld r30,X+
	ld r31,X
	sbiw r26,1
	ld r18,Z
	adiw r26,4
	ld r24,X
	eor r24,r18
	st Z,r24
	ret
	.size	_ZN4Cled10LED_invertEv, .-_ZN4Cled10LED_invertEv

	.section	.text._ZN4Cled11flash_timerEPiPhhi,"ax",@progbits
.global	_ZN4Cled11flash_timerEPiPhhi
	.type	_ZN4Cled11flash_timerEPiPhhi, @function
_ZN4Cled11flash_timerEPiPhhi:
//==> void Cled::flash_timer(int time[], uint8_t status[], uint8_t array_size, int reset_time) {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
	ret
	.size	_ZN4Cled11flash_timerEPiPhhi, .-_ZN4Cled11flash_timerEPiPhhi

	.section	.text.main,"ax",@progbits
.global	main
	.type	main, @function
main:
//==> int main(void) {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	Red.init();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled4initEv
//==> 	Red.LED_on();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled6LED_onEv
//==> 	Red.LED_off();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled7LED_offEv
//==> 	Red.LED_invert();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled10LED_invertEv
//==> }
	ldi r24,0
	ldi r25,0
	ret
	.size	main, .-main

	.section	.text._GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_,"ax",@progbits
	.type	_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_, @function
_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_:
//==> }
	push r16
	push r17
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
//==> Cled Red(&PORTB, &DDRB, (1 << PB5), &PINB);
	ldi r16,lo8(35)
	ldi r17,0
	ldi r18,lo8(32)
	ldi r20,lo8(36)
	ldi r21,0
	ldi r22,lo8(37)
	ldi r23,0
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4CledC1EPVhS1_hS1_
/* epilogue start */
//==> }
	pop r17
	pop r16
	ret
	.size	_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_, .-_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_
	.global __do_global_ctors

	.section .ctors,"a",@progbits
	.p2align	1
	.word	gs(_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_)
.global	Red

	.section	.bss.Red,"aw",@nobits
	.type	Red, @object
	.size	Red, 7
Red:
	.zero	7

As it's "Debug" I left taht at the default -O1 setting. I don't detect a whole lot of difference if I set that to -Os in fact:

	.text
.Ltext0:

	.section	.text._ZN4Cled6statusEv,"ax",@progbits
.global	_ZN4Cled6statusEv
	.type	_ZN4Cled6statusEv, @function
_ZN4Cled6statusEv:
//==> uint8_t Cled::status() {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	if ((*mPin & mMask) != 0) {
	movw r26,r24
	adiw r26,5
	ld r30,X+
	ld r31,X
	sbiw r26,5+1
	ld r18,Z
	adiw r26,4
	ld r24,X
	mov r25,r18
	and r25,r24
	ldi r24,lo8(1)
	brne .L2
	ldi r24,0
.L2:
//==> }
	ret
	.size	_ZN4Cled6statusEv, .-_ZN4Cled6statusEv

	.section	.text._ZN4Cled7LED_offEv,"ax",@progbits
.global	_ZN4Cled7LED_offEv
	.type	_ZN4Cled7LED_offEv, @function
_ZN4Cled7LED_offEv:
//==> void Cled::LED_off() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	if (status() == ON) {
	call _ZN4Cled6statusEv
	cpi r24,lo8(1)
	brne .L3
//==> 		*mPort &= ~mMask;
	ld r30,Y
	ldd r31,Y+1
	ld r25,Z
	ldd r24,Y+4
	com r24
	and r24,r25
	st Z,r24
.L3:
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled7LED_offEv, .-_ZN4Cled7LED_offEv

	.section	.text._ZN4Cled4initEv,"ax",@progbits
.global	_ZN4Cled4initEv
	.type	_ZN4Cled4initEv, @function
_ZN4Cled4initEv:
//==> void Cled::init() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	LED_off();
	call _ZN4Cled7LED_offEv
//==> 	*mDdr |= mMask;
	ldd r30,Y+2
	ldd r31,Y+3
	ld r25,Z
	ldd r24,Y+4
	or r24,r25
	st Z,r24
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled4initEv, .-_ZN4Cled4initEv

	.section	.text._ZN4CledC2EPVhS1_hS1_,"ax",@progbits
.global	_ZN4CledC2EPVhS1_hS1_
	.type	_ZN4CledC2EPVhS1_hS1_, @function
_ZN4CledC2EPVhS1_hS1_:
//==> Cled::Cled(volatile uint8_t* pPort, volatile uint8_t* pDdr, volatile uint8_t mask, volatile uint8_t* pPin) :
	push r16
	push r17
	push r28
	push r29
	push __zero_reg__
	in r28,__SP_L__
	in r29,__SP_H__
/* prologue: function */
/* frame size = 1 */
/* stack size = 5 */
.L__stack_usage = 5
	std Y+1,r18
//==> mPort(pPort), mDdr(pDdr), mMask(mask), mPin(pPin)
	movw r30,r24
	std Z+1,r23
	st Z,r22
	std Z+3,r21
	std Z+2,r20
	ldd r18,Y+1
	std Z+4,r18
	std Z+6,r17
	std Z+5,r16
//==> 	init();
	call _ZN4Cled4initEv
/* epilogue start */
//==> }
	pop __tmp_reg__
	pop r29
	pop r28
	pop r17
	pop r16
	ret
	.size	_ZN4CledC2EPVhS1_hS1_, .-_ZN4CledC2EPVhS1_hS1_
.global	_ZN4CledC1EPVhS1_hS1_
	.set	_ZN4CledC1EPVhS1_hS1_,_ZN4CledC2EPVhS1_hS1_

	.section	.text._ZN4Cled6LED_onEv,"ax",@progbits
.global	_ZN4Cled6LED_onEv
	.type	_ZN4Cled6LED_onEv, @function
_ZN4Cled6LED_onEv:
//==> void Cled::LED_on() {
	push r28
	push r29
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
	movw r28,r24
//==> 	if (status() == OFF) {
	call _ZN4Cled6statusEv
	cpse r24,__zero_reg__
	rjmp .L7
//==> 		*mPort |= mMask;
	ld r30,Y
	ldd r31,Y+1
	ld r25,Z
	ldd r24,Y+4
	or r24,r25
	st Z,r24
.L7:
/* epilogue start */
//==> }
	pop r29
	pop r28
	ret
	.size	_ZN4Cled6LED_onEv, .-_ZN4Cled6LED_onEv

	.section	.text._ZN4Cled10LED_invertEv,"ax",@progbits
.global	_ZN4Cled10LED_invertEv
	.type	_ZN4Cled10LED_invertEv, @function
_ZN4Cled10LED_invertEv:
//==> void Cled::LED_invert() {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	*mPort ^= mMask;
	movw r26,r24
	ld r30,X+
	ld r31,X
	sbiw r26,1
	ld r18,Z
	adiw r26,4
	ld r24,X
	eor r24,r18
	st Z,r24
	ret
	.size	_ZN4Cled10LED_invertEv, .-_ZN4Cled10LED_invertEv

	.section	.text._ZN4Cled11flash_timerEPiPhhi,"ax",@progbits
.global	_ZN4Cled11flash_timerEPiPhhi
	.type	_ZN4Cled11flash_timerEPiPhhi, @function
_ZN4Cled11flash_timerEPiPhhi:
//==> void Cled::flash_timer(int time[], uint8_t status[], uint8_t array_size, int reset_time) {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
	ret
	.size	_ZN4Cled11flash_timerEPiPhhi, .-_ZN4Cled11flash_timerEPiPhhi

	.section	.text.main,"ax",@progbits
.global	main
	.type	main, @function
main:
//==> int main(void) {
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
//==> 	Red.init();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled4initEv
//==> 	Red.LED_on();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled6LED_onEv
//==> 	Red.LED_off();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled7LED_offEv
//==> 	Red.LED_invert();
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4Cled10LED_invertEv
//==> }
	ldi r24,0
	ldi r25,0
	ret
	.size	main, .-main

	.section	.text._GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_,"ax",@progbits
	.type	_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_, @function
_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_:
//==> }
	push r16
	push r17
/* prologue: function */
/* frame size = 0 */
/* stack size = 2 */
.L__stack_usage = 2
//==> Cled Red(&PORTB, &DDRB, (1 << PB5), &PINB);
	ldi r16,lo8(35)
	ldi r17,0
	ldi r18,lo8(32)
	ldi r20,lo8(36)
	ldi r21,0
	ldi r22,lo8(37)
	ldi r23,0
	ldi r24,lo8(Red)
	ldi r25,hi8(Red)
	call _ZN4CledC1EPVhS1_hS1_
/* epilogue start */
//==> }
	pop r17
	pop r16
	ret
	.size	_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_, .-_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_
	.global __do_global_ctors

	.section .ctors,"a",@progbits
	.p2align	1
	.word	gs(_GLOBAL__sub_I__ZN4CledC2EPVhS1_hS1_)
.global	Red

	.section	.bss.Red,"aw",@nobits
	.type	Red, @object
	.size	Red, 7
Red:
	.zero	7

Whatever way you look at it that is a HUGE amount of code to flash a fixed LED that might have been done in 10 opcodes or less.

 

People say C++ is "wasteful" but it's not the language as such - you could have written this to be much "tighter" - it's just that it encourages you to "abstract" things and when you do that you pay the penalty for "generic" code. So don't blame C++ as such but the way it's used. You can concoct a dog's dinner like this in plain C too if you want!

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

You can concoct a dog's dinner like this in plain C too if you want!

Indeed.  I can't even find the I/O accesses in that particular generated code.  IMO much chance for very hard to find RMW problems if there happens to be any other work on that port.

 

I can't find the past thread I was thinking of, where the C++ guru(s) posted a "decent"/non-objectionable generic I/O implementation.

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

I think that a proper implementation of C++ for the AVR would solve this type of thing. Rather than <avr/io.h> there should be a different set of headers that define singleton classes for ports, timers, etc. With this setup, registers like PORTD or TCCR1A would not be defined (at least in a way visible to the end user) and never accessed directly, only the classes would know that the registers exist. With this, access could be done as efficiently as possible. The end user would not need to worry about exceptions like PORTF as it would be handed by the device specific header, though it would need to be done in a way that the user knows that the port is exceptional since it has implications such as RMW. It would of course also have to be written in such a way that no code is generated for a class that is never used in the application. It would also prevent problems such as someone creating say, a 16 bit timer class, then creating multiple instances of it even though there is really only one 16 bit timer on the device.

Regards,
Steve A.

The Board helps those that help themselves.

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

It would of course also have to be written in such a way that no code is generated for a class that is never used in the application

That's something that surprised me about the above - nothing ever calls/access flash_timer yet the code for it is there. I thought -ffunction-sections and -gc-sections would have removed it - but maybe all the functions in a class are grouped together as far as those mechanisms are concerned?

 

EDIT: just checked other machine and -ffunction-sections was ticked but -gc-sections was unticked. Not sure if I'd been "playing" previously (trying to prevent code being discarded when setting up an example) or whether a C++ project in Studio defaults to this.

 

EDIT2: no, I enabled both but flash_timer code (well just "ret") remains. So presumably it just discards whole classes not functions within?

Last Edited: Tue. Jan 27, 2015 - 05:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry for the delay, just got home from work.  The flash_timer ain't even started, basically at the main code when it calls flash_timer function in a TIMER, it follows a pattern of blinks depends on the time and then resets once it gets to the end of the array.  This wasn't the best idea, but it worked.  If there a better way, I be happy to hear it.  Yes there's a total of 7 Switches/Dual Color Leds, so there would been 7 classes to control each.  The UI will be controlling those classes.

 

Now I seen

 

Cled::Cled(volatile uint8_t* pPort, volatile uint8_t* pDdr, volatile uint8_t mask, volatile uint8_t* pPin) :
	mPort(pPort), mDdr(pDdr), mMask(mask), mPin(pPin)

and I never knew you could do this to set variables within a class and I will take that.  It a bad habit of using this-> within a class and I understand it not required, so I am going to start removing them.  I always CAP my classes, I ain't got a clue why I started that.

Thanks
Shane

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

Yes there's a total of 7 Switches/Dual Color Leds, so there would been 7 classes to control each.

No, there will be 7 >>instances<<.

Regards,
Steve A.

The Board helps those that help themselves.

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

A bit of a follow up on the example code that Cliff had posted above.  It appears that the code bloat is mainly caused by the status routine and the volatile pointers used in the class definition.  In the case of LEDs, how often in real code would you check the status of an LED prior to turning it on or off?  I deleted the LED status checks in LED_on() and LED_off() and also deleted status().

 

This is some code I threw together modified and based on Cliff's example above that shows that the code bloat can be controlled.  This code is written for -std=c++11 and is optimized at -O1.  It is late and I am tired, that's my excuse if I effed up. ;)

/*
 * C__CledClass.cpp
 *
 * Created: 1/27/2015 2:29:32 PM
 *  Author: Larry
 *  modified version of clawson's example code
 *  -std=c++11
*/ 

#define F_CPU 16000000UL
#include <util/delay.h>
#include <avr/io.h>


#include "CledClass.h"

int main(void) {

 Cled Red(PORTB, DDRB, PINB, PINB5);
 Cled Blue(PORTB, DDRB, PINB, PINB4);

 Red.LED_on();
 Red.LED_off();

 Blue.LED_on();
 Blue.LED_off();

 while(1) {
  Red.LED_invert();
  Blue.LED_invert();
  _delay_ms(100);
 }

}
/*
* CledClass.h
*
* Created: 1/27/2015 4:20:22 PM
*  Author: Larry
*  modified version of clawson's example code
*  -std=c++11
*/

#ifndef CLEDCLASS_H_
#define CLEDCLASS_H_

#define ON 1
#define OFF 0

class Cled {

public:

 Cled(volatile uint8_t &Portx, volatile uint8_t &Ddrx, volatile uint8_t &Pinx, volatile uint8_t Pin) : mPort(&Portx), mDdr(&Ddrx), mPincrl(&Pinx), mPin(Pin)
 {
  init();
 };

 void init() {
  //Let's Make Sure Our LED is Turned Off
  LED_off();
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
 }

 void LED_off() {
  *mPort &= ~(1 << mPin);
 }

 void LED_on() {
  *mPort |= (1 << mPin);
 }

 void LED_invert() {
  *mPincrl = (1 << mPin);
//  *mPort ^= (1 << mPin);
 }

 void flash_timer(int time[], uint8_t status[], uint8_t array_size, int reset_time) {

 }

private:

 volatile uint8_t * mPort;
 volatile uint8_t * mDdr;
 volatile uint8_t * mPincrl;
 uint8_t mPin;
 
};

#endif /* CLEDCLASS_H_ */

C++CledClass.elf:     file format elf32-avr

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         000000b0  00000000  00000000  00000054  2**1
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .comment      00000030  00000000  00000000  00000104  2**0
                  CONTENTS, READONLY
  2 .debug_aranges 00000020  00000000  00000000  00000134  2**0
                  CONTENTS, READONLY, DEBUGGING
  3 .debug_info   000004f8  00000000  00000000  00000154  2**0
                  CONTENTS, READONLY, DEBUGGING
  4 .debug_abbrev 000001bf  00000000  00000000  0000064c  2**0
                  CONTENTS, READONLY, DEBUGGING
  5 .debug_line   00000154  00000000  00000000  0000080b  2**0
                  CONTENTS, READONLY, DEBUGGING
  6 .debug_frame  00000024  00000000  00000000  00000960  2**2
                  CONTENTS, READONLY, DEBUGGING
  7 .debug_str    00000288  00000000  00000000  00000984  2**0
                  CONTENTS, READONLY, DEBUGGING
  8 .debug_ranges 00000010  00000000  00000000  00000c0c  2**0
                  CONTENTS, READONLY, DEBUGGING

Disassembly of section .text:

00000000 <__vectors>:
   0: 0c 94 34 00  jmp 0x68 ; 0x68 <__ctors_end>
   4: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
   8: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
   c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  10: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  14: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  18: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  1c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  20: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  24: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  28: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  2c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  30: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  34: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  38: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  3c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  40: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  44: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  48: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  4c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  50: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  54: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  58: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  5c: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  60: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>
  64: 0c 94 3e 00  jmp 0x7c ; 0x7c <__bad_interrupt>

00000068 <__ctors_end>:
  68: 11 24        eor r1, r1
  6a: 1f be        out 0x3f, r1 ; 63
  6c: cf ef        ldi r28, 0xFF ; 255
  6e: d8 e0        ldi r29, 0x08 ; 8
  70: de bf        out 0x3e, r29 ; 62
  72: cd bf        out 0x3d, r28 ; 61
  74: 0e 94 40 00  call 0x80 ; 0x80 <main>
  78: 0c 94 56 00  jmp 0xac ; 0xac <_exit>

0000007c <__bad_interrupt>:
  7c: 0c 94 00 00  jmp 0 ; 0x0 <__vectors>

00000080 <main>:
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
 }

 void LED_off() {
  *mPort &= ~(1 << mPin);
  80: 90 e2        ldi r25, 0x20 ; 32
  82: 2d 98        cbi 0x05, 5 ; 5

 void init() {
  //Let's Make Sure Our LED is Turned Off
  LED_off();
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
  84: 25 9a        sbi 0x04, 5 ; 4
 }

 void LED_off() {
  *mPort &= ~(1 << mPin);
  86: 80 e1        ldi r24, 0x10 ; 16
  88: 2c 98        cbi 0x05, 4 ; 5

 void init() {
  //Let's Make Sure Our LED is Turned Off
  LED_off();
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
  8a: 24 9a        sbi 0x04, 4 ; 4
 void LED_off() {
  *mPort &= ~(1 << mPin);
 }

 void LED_on() {
  *mPort |= (1 << mPin);
  8c: 2d 9a        sbi 0x05, 5 ; 5
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
 }

 void LED_off() {
  *mPort &= ~(1 << mPin);
  8e: 2d 98        cbi 0x05, 5 ; 5
 }

 void LED_on() {
  *mPort |= (1 << mPin);
  90: 2c 9a        sbi 0x05, 4 ; 5
  //Set Our LED To Output
  *mDdr |= (1 << mPin);
 }

 void LED_off() {
  *mPort &= ~(1 << mPin);
  92: 2c 98        cbi 0x05, 4 ; 5
 void LED_on() {
  *mPort |= (1 << mPin);
 }

 void LED_invert() {
  *mPincrl = (1 << mPin);
  94: 93 b9        out 0x03, r25 ; 3
  96: 83 b9        out 0x03, r24 ; 3
 #else
  //round up by default
  __ticks_dc = (uint32_t)(ceil(fabs(__tmp)));
 #endif

 __builtin_avr_delay_cycles(__ticks_dc);
  98: 2f ef        ldi r18, 0xFF ; 255
  9a: 31 ee        ldi r19, 0xE1 ; 225
  9c: 44 e0        ldi r20, 0x04 ; 4
  9e: 21 50        subi r18, 0x01 ; 1
  a0: 30 40        sbci r19, 0x00 ; 0
  a2: 40 40        sbci r20, 0x00 ; 0
  a4: e1 f7        brne .-8       ; 0x9e <main+0x1e>
  a6: 00 c0        rjmp .+0       ; 0xa8 <main+0x28>
  a8: 00 00        nop
  aa: f4 cf        rjmp .-24      ; 0x94 <main+0x14>

000000ac <_exit>:
  ac: f8 94        cli

000000ae <__stop_program>:
  ae: ff cf        rjmp .-2       ; 0xae <__stop_program>

 

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Wed. Jan 28, 2015 - 06:41 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

Oh style.

 


#include <stdbool.h>
// put all these #defines in the .h for the hardware config
#define LED1_DDR  DDRD
#define LED1_PORT PORTD
#define LED1_BITNO 3
#define LED1_ON  (LED1_PORT |= (1<<LED1_BITNO))  // depends on LED anode/cathode wiring. Reverse _ON and _OFF
#define LED1_OFF (LED1_PORT &= ~(1<<LED1_BITNO)) 
// re above: do what  you can at compile time, not run time!

void inline LED1set(bool state)  {
    state ? LED1_ON : LED1_OFF;
}

main()  {
    LED1_DDR |= (1<<LED1_BITNO);
    LED1set(true);
}

 

You can C++ class this, but LEDs are too simple to need it, and they depend on wiring - which would/should be in the class constructor. I hate constructors for embedded.

 

Last Edited: Wed. Jan 28, 2015 - 09:00 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

larryvc: I deleted the LED status checks in LED_on() and LED_off() and also deleted status()...

...and then you ended up with  separate code for each instance, boiled down to a cbi (for LED_off())?  Looks good but I sure don't know how it really works in practice.  The shown routines seem to be that single instruction, back-to-back-to-back--how will that

  8a: 24 9a        sbi 0x04, 4 ; 4
  8c: 2d 9a        sbi 0x05, 5 ; 5
  8e: 2d 98        cbi 0x05, 5 ; 5
  90: 2c 9a        sbi 0x05, 4 ; 5
  92: 2c 98        cbi 0x05, 4 ; 5
  94: 93 b9        out 0x03, r25 ; 3
  96: 83 b9        out 0x03, r24 ; 3

be used?  In a real generic case with port and pin not known at compile time, how can the shown instructions be generated?

 

 

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

In a real generic case with port and pin not known at compile time, how can the shown instructions be generated?

They can't (that's essentially what my trimmed version of OPs code shows though I agree with Larry that the status checking thing was over-kill and over-engineering). This is equally true in C. You simply cannot get any compiler: C or C++, to generate SBIs and CBIs for something that is only known at run time.

 

(unless you have one CBI and one SBI routine for every potential LED position and then just picked the right one to call at run time).

 

BTW nice use of PIN toggling by Larry there - at first I thought the implementation of:

 void LED_invert() {
  *mPincrl = (1 << mPin);
//  *Port ^= (1 << mPin);
 }

was wrong and was expecting to find the code that is commented but then I realised this was referring to PIN not PORT so that explains how one OUT (or R24 or R25) is being used to do the toggling in the while() loop.

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

I certainly hope that Shane (madcitysw) does not feel as though we have hijacked his thread.  There is a lot that can be learned here.

 

Cliff, the code generated for the commented out line that you noted above is: (note that *Port s/b *mPort - corrected in my post above)

 void LED_invert() {
//  *mPincrl = (1 << mPin);
  *mPort ^= (1 << mPin);
  94: 85 b1        in r24, 0x05 ; 5
  96: 82 27        eor r24, r18
  98: 85 b9        out 0x05, r24 ; 5
  9a: 85 b1        in r24, 0x05 ; 5
  9c: 89 27        eor r24, r25
  9e: 85 b9        out 0x05, r24 ; 5

and I suppose that this is more correct as it would work even with the older chips that don't have the ability to toggle with a write to PIN.

 

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Wed. Jan 28, 2015 - 07:46 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
...how will that be used?  In a real generic case with port and pin not known at compile time, how can the shown instructions be generated?

Maybe I am missing out on something, said the somewhat naïve embedded programmer.  I have never had a case where I did not know what was tied to the ports prior to compile time.

 

Can you give an example where this applies?  

"I may make you feel but I can't make you think" - Jethro Tull - Thick As A Brick

"void transmigratus(void) {transmigratus();} // recursio infinitus" - larryvc

"It's much more practical to rely on the processing powers of the real debugger, i.e. the one between the keyboard and chair." - JW wek3

"When you arise in the morning think of what a privilege it is to be alive: to breathe, to think, to enjoy, to love." -  Marcus Aurelius

Last Edited: Wed. Jan 28, 2015 - 07:57 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

larryvc wrote:

 

theusch wrote:

...how will that be used?  In a real generic case with port and pin not known at compile time, how can the shown instructions be generated?

 

Maybe I am missing out on something, said the somewhat naïve embedded programmer.  I have never had a case where I did not know what was tied to the ports prior to compile time.

 

Can you give an example where this applies?  

 

I agree. I see a lot of what I call overly-generalized code to do run-time decisions on things that are soldered down.

Generalization has its place but brings complexity and obtuseness. I see way too much overuse of this in C++, to include function and esp.  operator overloads where it looks cute but 6 months later, its hidden magic is a killer for sustaining.

 

Last Edited: Wed. Jan 28, 2015 - 08:01 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

larryvc wrote:

I certainly hope that Shane (madcitysw) does not feel as though we have hijacked his thread.  There is a lot that can be learned here.

 

 

Oh no, I don't feel as you guys hijacked this thread, it about learning the mistakes on this thread and nothing wrong with that.  Right now I reading everyone posts and trying to get some sense out of them... 

Thanks
Shane