More efficient code - maybe

Go To Last Post
82 posts / 0 new

Pages

Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Split from:

https://www.avrfreaks.net/forum/...

 

 

I learned this:

  for(i=0;i<32;i++){
  if((TMR1 < 700) && (TMR1 > 400))
     ir_code_INT &= ~(1<<(31 -i));
  else if((TMR1 < 2000) && (TMR1 > 1000))
     ir_code_INT |= (1<<(31 -i));}

 

And this is more efficient with changing the for loop direction and taking out the extra subtraction operations:

  for(i=31;i>=0;i--){
  if((TMR1 < 700) && (TMR1 > 400))
     ir_code_INT &= ~(1<<(i));
  else if((TMR1 < 2000) && (TMR1 > 1000))
     ir_code_INT |= (1<<(i));}

Last Edited: Sat. Aug 18, 2018 - 11:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wolfrose wrote:
And this is more efficient ...

What does that post/comment have to do with the subject of the thread?

 

This is where I sent you, when you had not initialized your timer in an Arduino environment.  Some of the links above will follow up on that.

 

Re your posted loops ("spot the difference?"):

1.  I'd probably use a walking mask to avoid having the compiler be tempted to do a real shift.

2.  If really doing timer work, then I'd probably use timer facilities.

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

theusch wrote:
What does that post/comment have to do with the subject of the thread?

Nothing that I can see!

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
(1<<(i));

The code can be made more efficient by eliminating the repeated variable shifts. The AVR can only shift 1 bit in a byte per instruction. If it were an ARM, it can shift n <32 bits in one instruction. 

With any form of optimisation, you should measure the execution time first to give a baseline value. You can compare any further optimisations with the value to determine if you have actually made an improvement. Also note that execution time may not be the only goal of optimisation - code size might be another. Again, measure first, optimise later. It's also worth consider the actual value of any optimisation - if the code executes once per second and you make it 2 microseconds faster, have you really gained anything useful? I tend to work towards having simple, readable code with minimal defects. Having highly optimised code with defects is not useful.

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

To avoid the variable shift in situations like this, I use a variable initialized outside the loop with 1, for example, then shift it in each iteration. It can also be used to terminate the loop:

 

uint32_t ir_code_INT = 0;
uint32_t current_bit = 1;
while(current_bit){
        if((TMR1 < 2000) && (TMR1 > 1000))
                ir_code_INT |= current_bit;
        current_bit <<= 1;
}

 

I suppose the bit can only be zero or one, so I'm just checking for ones, since the variable was already initialized with all zeroes.

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

theusch wrote:

wolfrose wrote:

And this is more efficient ...

 

What does that post/comment have to do with the subject of the thread?

 

This is where I sent you, when you had not initialized your timer in an Arduino environment.  Some of the links above will follow up on that.

 

Re your posted loops ("spot the difference?"):

1.  I'd probably use a walking mask to avoid having the compiler be tempted to do a real shift.

2.  If really doing timer work, then I'd probably use timer facilities.

 

Oh, I'm sorry I thought I'm commenting something that could be related. I wanted to connect something related to the main post of the thread, but also I knew that my comment may not be as professional as it should be :)

 

I know you sent me the link to follow the comments and learn from them. So, my comment changed to be a new thread :) that's an honor for me that my comment has a value to be a subject to a new thread.

 

=============================================================

 

For your reply to my comment:

1. The walking mask is also a good idea, I applied that method in a function I developed to convert HEX value to binary, it was one of my practices in C.

 

uint32_t send_data(uint32_t data)
{
   uint8_t i,data_cpy,shifter=0x80000000;

   for (i = 0; i < 32; i++)
   {
       if (data & 0x80000000)
       {
           printf("1");
           data_cpy |= shifter;
       }
       else
       {
           printf("0");
           data_cpy &= ~shifter;
       }

       data <<= 1;
       shifter >>=1;
   }
   printf("\n");
    return data_cpy;
}

 

2. Like what facilities?

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

Kartman wrote:

(1<<(i));

The code can be made more efficient by eliminating the repeated variable shifts. The AVR can only shift 1 bit in a byte per instruction. If it were an ARM, it can shift n <32 bits in one instruction. 

With any form of optimisation, you should measure the execution time first to give a baseline value. You can compare any further optimisations with the value to determine if you have actually made an improvement. Also note that execution time may not be the only goal of optimisation - code size might be another. Again, measure first, optimise later. It's also worth consider the actual value of any optimisation - if the code executes once per second and you make it 2 microseconds faster, have you really gained anything useful? I tend to work towards having simple, readable code with minimal defects. Having highly optimised code with defects is not useful.

That's really interesting to know about ARM microcontrollers. OK, I read at the beginning of the Atmega328p that it has 32 working registers, what does that mean exactly? Is that related to the concept you mentioned about the ARM microcontrollers?

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

The AVR is a RISC architecture machine. This implies it can only do arithmetic and logical operations using its internal registers of which it has 32 of 8bit. The ARM, also being RISC architecture has 16, 32bit registers.

This has little to do with the shifting. The ARM has what is called a 'barrel shifter' which is a piece of hardware logic that can shift/rotate a 32 bit register by n<32 bits. The AVR does not have this and can only shift one bit at a time. Consider what the AVR has to do to create your 32bit bitmask. You can cheat and look at the assembler code the compiler generates or step through the assembler in the debugger. This should give you a feel for what is being performed. Compare this to the code in #5. 

 

Sometimes it does help to understand the architecture of the microcontroller when writing the code.

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

I't not clear to me what you actually want to do!

 

It look's to me that you want 3 levels of a bit! 

 

Do you have a init values that matter? 

 

normally for things like this you clear all bits (or set all).

and then make a loop that change those that need it (and do nothing with the rest they are allready ok)

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

wolfrose wrote:

  for(i=0;i<32;i++){
  if((TMR1 < 700) && (TMR1 > 400))
     ir_code_INT &= ~(1<<(31 -i));
  else if((TMR1 < 2000) && (TMR1 > 1000))
     ir_code_INT |= (1<<(31 -i));}

Apparently this is for a toolchain with 32-bit ints.

How is it different from

if(TMR1< 700 && TMR1> 400) {
    ir_code_INT=0;
} else if(TMR1< 2000 && TMR1> 1000) {
    ir_code_INT= -1;
}

?

Iluvatar is the better part of Valar.

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

Kartman wrote:

​The ARM has what is called a 'barrel shifter' which is a piece of hardware logic that can shift/rotate a 32 bit register by n<32 bits.

Does than mean, 32-bits locations, I think it's a set or clear any bit in 32-bits register, or you mean n<32 bit masks? 

 

Quote:

The AVR does not have this and can only shift one bit at a time. Consider what the AVR has to do to create your 32bit bitmask. You can cheat and look at the assembler code the compiler generates or step through the assembler in the debugger. This should give you a feel for what is being performed. Compare this to the code in #5.

Yes, I liked the code in #5. I also posted similar code in #6. But does it differ from my main code in #1 in efficiency? 

 

 

Quote:

Sometimes it does help to understand the architecture of the microcontroller when writing the code.

Absolutely! I like to understand the internal architecture, currently I'm focusing on writing codes and going through the sensors and modules I have, but most of the time I have to get a general idea of the hardware.

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

skeeve wrote:

wolfrose wrote:

  for(i=0;i<32;i++){
  if((TMR1 < 700) && (TMR1 > 400))
     ir_code_INT &= ~(1<<(31 -i));
  else if((TMR1 < 2000) && (TMR1 > 1000))
     ir_code_INT |= (1<<(31 -i));}

Apparently this is for a toolchain with 32-bit ints.

How is it different from

if(TMR1< 700 && TMR1> 400) {
    ir_code_INT=0;
} else if(TMR1< 2000 && TMR1> 1000) {
    ir_code_INT= -1;
}

?

 

The first one is setting and clearing specific bits, the second one is assigning the variable to a certain value depending on the if conditions? 

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

sparrow2 wrote:

I't not clear to me what you actually want to do!

 

It look's to me that you want 3 levels of a bit! 

 

Do you have a init values that matter? 

 

normally for things like this you clear all bits (or set all).

and then make a loop that change those that need it (and do nothing with the rest they are allready ok)

Of course, this is most like my code:

	for (i=0;i<5;i++)
    {
		for (j=7;j>=0;j--)
		{
			while(!(PINB & (1<<PINB0)));                // wait for 0 leading bit
			TCNT0=0;
			while(PINB & (1<<PINB0));                   // high bit 0 or 1 
			TMR_TEMP = TMR0_MSK;
			if (TMR_TEMP < 35 && TMR_TEMP > 20)
				DECODE_TEMP &= ~(1<<(j));
			else
				DECODE_TEMP |= (1<<(j));
		}
		bits_timing[i] = DECODE_TEMP;
    }

I learned this from a similar AVR code for the same sensor. At this link:

http://davidegironi.blogspot.com/2012/12/reading-temperature-and-humidity-on-avr.html#.W3mp2OgzZPY

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

skeeve wrote:
Apparently this is for a toolchain with 32-bit ints.

How did you divine that?

wolfrose wrote:
I learned this from a similar AVR code for the same sensor.

snipe hunt

noun

noun: snipe hunt; plural noun: snipe hunts

  1. 1.

    an act of hunting wild snipe.

    "a dog is helpful on a snipe hunt, especially for retrieving"

  2. 2.

    US

    a practical joke in which an unwitting victim is sent in pursuit of something that doesn't exist.

    "one or two gullible youths are selected to participate in the notorious snipe hunt"

    • a foolish or hopeless search for or pursuit of something unattainable; a wild goose chase

Top definition

 

snipe hunt

A North-American prank and rite of passage wherein older adolescents take younger boys into the wilderness for the supposed purpose of “snipe hunting.” Snipes are an imaginary game bird purported to resemble quails or pheasants or what have you (the fictional snipe is not to be confused with the extant North American shorebird of that same name). Snipe hunts take place on moonless nights; the victims are provided burlap bags with which to catch the birds, while the conspirators spot them with flashlights. The conspirators make birdcalls, through rocks in the bushes, and urgently cry out “snipe” to make the victims believe that there are actually birds in the area. The victims don’t want to be the only one who can’t see the imaginary birds, so they claim to have seen them also. Pretty soon the victims have convinced each other they are surrounded by snipes and proceed to run about foolishly in search of the non-existent birds. “Dude right there didn’t you see it?” The conspirators will often agree that they have just seen a snipe in that cactus patch or lake or thorny bush and order then the victims to dive in and catch it with his respective sack. The victims are then often abandoned by their guides, thus completing the joke. The cycle repeats when this year’s dupes become privy to the joke and then take their younger brothers out the following year, in search of the ever-illusive snipe.

"You've never heard of snipe hunting? Dude we should go this weekend."

 

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

Of course, this is most like my code:

That is not an answer to my question!

 

your code clear bit if ]400..700[

it set bit if ]1000..2000[

 

so if it's 800 it keep the bit as it is is that really what you want?

the code in #13 don't have that problem.

 

and as i say you normally don't do it that way, and a quick look a the lib you point don't do it either (it does it for flipping a port pin but that is different and make sense). 

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

theusch wrote:
skeeve wrote:
Apparently this is for a toolchain with 32-bit ints.

How did you divine that?

From 1<<31, int must be at least 32 bits.

Iluvatar is the better part of Valar.

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

theusch wrote:
Top definition

snipe hunt

A North-American prank and rite of passage wherein older adolescents take younger boys into the wilderness for the supposed purpose of “snipe hunting.” Snipes are an imaginary game bird purported to resemble quails or pheasants or what have you (the fictional snipe is not to be confused with the extant North American shorebird of that same name). Snipe hunts take place on moonless nights; the victims are provided burlap bags with which to catch the birds, while the conspirators spot them with flashlights. The conspirators make birdcalls, through rocks in the bushes, and urgently cry out “snipe” to make the victims believe that there are actually birds in the area. The victims don’t want to be the only one who can’t see the imaginary birds, so they claim to have seen them also. Pretty soon the victims have convinced each other they are surrounded by snipes and proceed to run about foolishly in search of the non-existent birds. “Dude right there didn’t you see it?” The conspirators will often agree that they have just seen a snipe in that cactus patch or lake or thorny bush and order then the victims to dive in and catch it with his respective sack. The victims are then often abandoned by their guides, thus completing the joke. The cycle repeats when this year’s dupes become privy to the joke and then take their younger brothers out the following year, in search of the ever-illusive snipe.

"You've never heard of snipe hunting? Dude we should go this weekend."

Fraser Crane was the best snipe-hunter of all time.

Iluvatar is the better part of Valar.

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

Kartman wrote:

The ARM has what is called a 'barrel shifter' which is a piece of hardware logic that can shift/rotate a 32 bit register by n<32 bits. The AVR does not have this and can only shift one bit at a time. Consider what the AVR has to do to create your 32bit bitmask. You can cheat and look at the assembler code the compiler generates or step through the assembler in the debugger. This should give you a feel for what is being performed. Compare this to the code in #5. 

 

Sometimes it does help to understand the architecture of the microcontroller when writing the code.

Any decent uC programmer who has spent time working with 8-bit devices would cringe at seeing things like (1 << (31 - i)), especially in a loop, knowing the amount of register churning this would involve on an 8-bit architecture.  And yet now with ARM (and other 32-bit families?), it only involves one subtraction instruction and one shift instruction.  That's a huge paradigm shift.  That and the easy and fast use of 32-bit values for integer scaling are among the things that attract me to the tiny 32-bit ARMs.

Last Edited: Mon. Aug 20, 2018 - 06:35 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wolfrose wrote:
The first one is setting and clearing specific bits, the second one is assigning the variable to a certain value depending on the if conditions?
The if-condition does not change.

Either 32 bits are set to 1 or 32 bits are cleared to 0.

If the target is not 32-bits, it deserves a comment.

With a 32-bit signed int, 1<<31 might be implementation-defined

and definitely deserves a comment.

 

Iluvatar is the better part of Valar.

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

The local variety of snipe here in Portugal is called "gambozino".

 

Image result for gambozinos

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

skeeve wrote:
Fraser Crane was the best snipe-hunter of all time.

I understand the reference to the Cheers TV show episode.  It indeed had most of the elements of the quoted description.

 

Now you are going to make me look it up -- I seem to recall that Frasier had the last laugh?

 

When on a primarily grouse hunting trip, we did see an occasional snipe -- "timber doodle" -- and pursued them for a session.  Got a couple shots, but too elusive to bag.  [pun intended]

 

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

theusch wrote:
skeeve wrote:
Fraser Crane was the best snipe-hunter of all time.

I understand the reference to the Cheers TV show episode.  It indeed had most of the elements of the quoted description.

 

Now you are going to make me look it up -- I seem to recall that Frasier had the last laugh?

That is what made him the best.

Iluvatar is the better part of Valar.

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

sparrow2 wrote:

Of course, this is most like my code:

That is not an answer to my question!

 

your code clear bit if ]400..700[

it set bit if ]1000..2000[

 

so if it's 800 it keep the bit as it is is that really what you want?

the code in #13 don't have that problem.

 

and as i say you normally don't do it that way, and a quick look a the lib you point don't do it either (it does it for flipping a port pin but that is different and make sense). 

 

Yes, because there are 2 bits lengths involved in this timing ranges, the 0 bit is between 400-700 about 561, bit 1 is between 1000-2000 about 1631. All in 1us resolution, the ranges could be narrower than this.

They could be 500-600 for 561us 0 bit and 1500-1800 for 1631us 1 bit. But I think there're some fluctuations in the timings sometimes, that's why I put more wide ranges.

 

 

Yes, the code in #13 is for DH11 sensor, the code in #1 is for the VS1838b sensor.

 

But I think both codes are similar in the structure.

 

The code in #13 doesn't have another if condition for the HIGH bit, I think that's the main difference.

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

This is also another function I've done on my PIC18F46k22 for DH11 sensor.

 

uint8_t DH11_read_8bits(void)
{
    int8_t i,k;
    DH11_code = 0;
    for (i=7;i>=0;i--)
    {   
        while(!PORTCbits.RC3);                  // wait for 0 leading bit
        TMR2 = 0;
        while(PORTCbits.RC3);                   // high bit 0 or 1 
        TMR2_temp = TMR2;
        if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
        DH11_code &= ~(1<<(i));
        else
        DH11_code |= (1<<(i));
    }
    return DH11_code;
}

 

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

The question why why would you do this:

      if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
        DH11_code &= ~(1<<(i));
        
        
        // for readability, it should be written like this:
        
    if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
        {
        DH11_code &= ~(1<<(i));
        }
        
        //note the use of { }. Why? Read the MISRA standard and other sources

DH11_code starts off as 0, so why clear bits? You only need to set them.

 

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

Kartman wrote:

The question why why would you do this:

      if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
        DH11_code &= ~(1<<(i));
        
        
        // for readability, it should be written like this:
        
    if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
        {
        DH11_code &= ~(1<<(i));
        }
        
        //note the use of { }. Why? Read the MISRA standard and other sources

 

 

When I started learning C, I learned that if it's only one instruction after a for or while, then there's no need for the curly braces, if more then curly braces are required. I feel it's more compact for me.

 

But also I can do it like this:

    if((TMR2_temp < 30) && (TMR2_temp > 20))// 0 bit is 26-28us
    {DH11_code &= ~(1<<(i));}

 

 

DH11_code starts off as 0, so why clear bits? You only need to set them.

You're right :) 

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

I was prompting you to do some learning.  

 

Also, how would you change that piece of code to use less shift operations? Currently, the first time through it does 7 shifts, next time 6 shifts and so on. How about one shift per iteration? This is for your learning, not mine.

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

other that the first code need 3 values of a bit to work!

either 0 and 1 , and then a value (or break because it an error) if it's not inside (like 800).

 

 

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

Kartman wrote:
DH11_code starts off as 0, so why clear bits? You only need to set them.

I tried this method, didn't work with 2 for loops I don't know why actually, but this version worked:

uint8_t DH11_read_8bits(uint8_t *bits_timing)
{
    int8_t i,k;
    DH11_code = 0;
    for (k=0;k<5;k++)
    {
        for (i=7;i>=0;i--)
        {   
            while(!PORTDbits.RD1);                  // wait for 0 leading bit
            TMR1 = 0;
            while(PORTDbits.RD1);                   // high bit 0 or 1 
            TMR1_temp = TMR1;
            if((TMR1_temp < 80) && (TMR1_temp > 60))// 0 bit is 26-28us
            DECODE_TEMP &= ~(1<<(i));
            else
            DECODE_TEMP |= (1<<(i));
        }
        bits_timing[k]=DECODE_TEMP;
    }
}

Kartman wrote:
How about one shift per iteration?

Tried this one but didn't work:

uint8_t DH11_read_8bits(uint8_t *bits_timing)
{
    int8_t i,k;uint8_t shifter=0x80;
    DH11_code = 0;
    for (k=0;k<5;k++)
    {
        for (i=7;i>=0;i--)
        {   
            while(!PORTDbits.RD1);                  // wait for 0 leading bit
            TMR1 = 0;
            while(PORTDbits.RD1);                   // high bit 0 or 1 
            TMR1_temp = TMR1;
            if((TMR1_temp < 30) && (TMR1_temp > 18))// 0 bit is 26-28us
            DECODE_TEMP &= ~shifter; 
            else
            DECODE_TEMP |= shifter;
            shifter>>=1;
        }
        bits_timing[k]=DECODE_TEMP;
    }

 

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

sparrow2 wrote:

other that the first code need 3 values of a bit to work!

either 0 and 1 , and then a value (or break because it an error) if it's not inside (like 800).

Then I have to expand the timer range, but I tested the timings, they are 561us for 0 and 1631us for 1, I need 16-bit timer for this implementation.

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

What happens to shifter after 8 shifts?

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

Kartman wrote:
What happens to shifter after 8 shifts?
Yes, you're right I missed this point, now it's working. This version saves 40 bytes of the program memory.

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

I've done 4 versions of the code, all compile 358 bytes for RAM:

1. 5 calls for 40-bits, and applying multiple variable shifts        ---------> compiles 8460 bytes

2. 5 calls for 40-bits, and applying variable shifter                   ---------> compiles 8428 bytes

3. 1 call for 40-bits, and applying multiple variable shifts         ---------> compiles 8440 bytes

4. 1 call for 40-bits, and applying variable shifter                    ---------> compiles 8408 bytes

 

This is the code for 40-bits, I'm just commenting and uncommenting for the shifter and multiple shifts strategy:

uint8_t DH11_read_8bits(uint8_t *bits_timing)
{
    int8_t i,k;//uint8_t shifter=0x80;
    DH11_code = 0;
    for (k=0;k<5;k++)
    {
//        shifter=0x80;
        for (i=7;i>=0;i--)
        {
            while(!PORTDbits.RD1);                  // wait for 0 leading bit
            TMR1 = 0;
            while(PORTDbits.RD1);                   // high bit 0 or 1
            TMR1_temp = TMR1;
            if((TMR1_temp < 30) && (TMR1_temp > 18))// 0 bit is 26-28us
            DH11_code &= ~(1<<(i));
            else
            DH11_code |= (1<<(i));
//            DH11_code &= ~shifter;
//            else
//            DH11_code |= shifter;
//            shifter>>=1;
        }
        bits_timing[k]=DH11_code;
    }
}

 

This is the code for 8-bits for each byte, I'm just commenting and uncommenting for the shifter and multiple shifts strategy:

uint8_t DH11_read_8bits(uint8_t *bits_timing)
{
    int8_t i,k;//uint8_t shifter=0x80;
    DH11_code = 0;
//        shifter=0x80;
        for (i=7;i>=0;i--)
        {
            while(!PORTDbits.RD1);                  // wait for 0 leading bit
            TMR1 = 0;
            while(PORTDbits.RD1);                   // high bit 0 or 1
            TMR1_temp = TMR1;
            if((TMR1_temp < 30) && (TMR1_temp > 18))// 0 bit is 26-28us
            DH11_code &= ~(1<<(i));
            else
            DH11_code |= (1<<(i));
//            DH11_code &= ~shifter;
//            else
//            DH11_code |= shifter;
//            shifter>>=1;
        }
    return DH11_code;
}

 

 

I've learned here more information about how to improve the code, but is this one aspect of code optimization? The size! Size is obvious in this example. How about speed? What are other possible aspects?

 

 

UPDATE:

After removing the 5 calls part, and applying shifter strategy, the code now compiles 356 RAM 8382 Program Memory.

Last Edited: Tue. Aug 21, 2018 - 05:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Another common aspect of optimization is memory management, how you access variables, etc.

For example, it's important to make sure constant data is kept in the flash memory area.

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

El Tangas wrote:
Another common aspect of optimization is memory management, how you access variables, etc.

For example, it's important to make sure constant data is kept in the flash memory area.

Flash access is slower and more restricted than SRAM access.

In some applications, that could be important.

Iluvatar is the better part of Valar.

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

First witch optimizing level are you using, if you aren't using -Os then try it!

 

I'm not sure how intelligent the compiler is but perhaps declare i inside the for loop, (to make sure that i only exist in a register) 

 

and since you insist on having a else on the if then at least remove : 

 DH11_code = 0;

  but I would  keep it and negate the result of the if and then remove the else part.

if you don't want that then do :

 DH11_code = 0xff;

and then remove the else part (but normally you clear and then set the needed bits)

 

add:

not "real" C (a GCC extention) make a switch and check for the interval using case 18 ... 30 : 

Last Edited: Tue. Aug 21, 2018 - 09:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Besides -Os, here are other useful optimizing flags:

-flto

-mrelax

 

These ones may or may not reduce code size (you have to test):

-fno-jump-tables

-fno-gcse

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

sparrow2 wrote:

First witch optimizing level are you using, if you aren't using -Os then try it!

I'm working now with MPLAB X, the warning level is 5.

 

If -Os is related to MPLAB x then where to include it?

 

Quote:

I'm not sure how intelligent the compiler is but perhaps declare i inside the for loop, (to make sure that i only exist in a register)

Yes, I'm doing this now in my current code sketch.

 

Quote:
and since you insist on having a else on the if then at least remove : 

 DH11_code = 0;

  but I would  keep it and negate the result of the if and then remove the else part.

if you don't want that then do :

 DH11_code = 0xff;

and then remove the else part (but normally you clear and then set the needed bits)

OK, I tried initializing DH11_code = 0xff; and keep only the clearing bits line, but didn't work.

 

uint8_t DH11_read_40bits(uint8_t *bits_timing)
{
    int8_t i,k;
    for (k=0;k<5;k++)
    {
        uint8_t shifter=0x80;DH11_code =0xff;
        for (i=7;i>=0;i--)
        {   
            while(!PORTCbits.RC3);                  // wait for 0 leading bit
            TMR2 = 0;
            while(PORTCbits.RC3);                   // high bit 0 or 1 
            TMR1_temp = TMR2;
            if((TMR1_temp < 30) && (TMR1_temp > 18))// 0 bit is 26-28us
            DH11_code &= ~shifter;
        bits_timing[k]=DH11_code;
    }
}

 

 

Quote:
add:

not "real" C (a GCC extention) make a switch and check for the interval using case 18 ... 30 : 

Yes, but I think that is more suitable with a lot of conditions to check for, this is only one check. But also thanks for the three dots punctuation for range checking, this is I guess I new skill with switch function.

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

don't bring PIC code and questions into the equation - this is an AVR site. Go to PIC forums for PIC related questions.

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

Yes, sorry. I'm also doing the same project with my Arduino uno board. It just the topic was about efficiency of a code, as I'm working on both platform.

 

And I'm happy I brought both my hardware setups to run this sensor code. But the code is the same for my AVR version:

 

The first sketch with the multiple shifts:

void DH11_read(uint8_t *bits_timing)
{
	uint8_t i,TMR_TEMP=0,DECODE_TEMP=0;int8_t j;
	DDRB = 0xFF;		// Pin0 output
	PORTB &= ~(1<<PB0);
	_delay_ms(18);
	PORTB |= (1<<PB0);
	DDRB = 0xFE;

	while((PINB & (1<<PINB0)));
	while(!(PINB & (1<<PINB0)));
	while((PINB & (1<<PINB0)));

	for (i=0;i<5;i++)
    {
		for (j=7;j>=0;j--)
		{
			while(!(PINB & (1<<PINB0)));                // wait for 0 leading bit
			TCNT0=0;
			while(PINB & (1<<PINB0));                   // high bit 0 or 1
			TMR_TEMP = TMR0_MSK;
			if (TMR_TEMP < 35 && TMR_TEMP > 20)
				DECODE_TEMP &= ~(1<<(j));
			else
				DECODE_TEMP |= (1<<(j));
		}
		bits_timing[i] = DECODE_TEMP;
    }
}

 

It compiles:

Sketch uses 3354 bytes (10%) of program storage space.
Global variables use 225 bytes (10%) of dynamic memory.

 

The second modified sketch:

void DH11_read(uint8_t *bits_timing)
{
	uint8_t TMR_TEMP=0,DECODE_TEMP=0;int8_t j,i;
	DDRB = 0xFF;		// Pin0 output
	PORTB &= ~(1<<PB0);
	_delay_ms(18);
	PORTB |= (1<<PB0);
	DDRB = 0xFE;

	while((PINB & (1<<PINB0)));
	while(!(PINB & (1<<PINB0)));
	while((PINB & (1<<PINB0)));

	for (i=0;i<5;i++)
    {	uint8_t shifter=0x80,DH11_code =0xff;
		for (j=7;j>=0;j--)
		{
			while(!(PINB & (1<<PINB0)));                // wait for 0 leading bit
			TCNT0=0;
			while(PINB & (1<<PINB0));                   // high bit 0 or 1
			TMR_TEMP = TMR0_MSK;
			if (TMR_TEMP < 35 && TMR_TEMP > 20)
				DH11_code &= ~(1<<(j));
			shifter>>=1;
		}
		bits_timing[i] = DH11_code;
    }
}

 

It compiles:

Sketch uses 3350 bytes (10%) of program storage space.
Global variables use 225 bytes (10%) of dynamic memory.

Last Edited: Wed. Aug 22, 2018 - 12:36 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

The idea was to get rid of the multiple shifts, yes? But they’ve snuck back in. So what are you wanting to demonstrate to us?

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

Yes, I agree, I got ride, I just wanted to post both codes.

 

Now it's OK, also I've got the advices about my code and suggestions to improve the code.

 

If you think the 2nd version of the code is good, then I have an idea of a good enhanced code, and how to develop a better functions. Thanks a lot,

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

You don’t need DH11_code and shifter. Chose one. To add a bit,
Shift then ‘or’ in the bit.
For msb first,
shifter<<=1;
If bit is one, then
shifter |= 1;

Efficient on just about any architecture.

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

you wrote in #38 :

OK, I tried initializing DH11_code = 0xff; and keep only the clearing bits line, but didn't work.

but that is what you ended up with in #40.

 

I think you have to many try and errors take a pen and paper and move the bits by hand! then single step it in the emulator.

 

Comments to :

If you think the 2nd version of the code is good

If it work's I guess it's ok but you overcomplicate the problem, (again single step the code).

 

But what is good code

fast

small

easy to read

 

and yours are none of the above (sorry to say).

 

if your goal is just small (you don't need speed, you wait most of the time anyway, so in real life you should have this code placed in an interrupt)

 

for smaller code try :

declare i and j inside the for loops to make it 100% clear for the compiler that they are only local. 

instead of shift try *2 or /2 depending of direction (because calculations in C has to be of int shifts sometimes are on 16 bit even for a 8 bit variable it don't seem to be a problem for * and / and it will still just make a shift).

you can get rid of the inner for loop(j) and make a while on shift (when done shift become 0 (false) ).

 

 

if you only use the function once make it inline (then you save the function call)

and if you can don't pass a pointer but use a global array.

But I guess we are down to less than max 20-50 bytes you can save unless you write it in ASM but that's an other day.

 

 

 

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

Kartman wrote:
You don’t need DH11_code and shifter. Chose one. To add a bit, Shift then ‘or’ in the bit. For msb first, shifter<<=1; If bit is one, then shifter |= 1; Efficient on just about any architecture.

How?

 

I tried this on CodeBlocks but it seems I didn't understand what you meant, sorry :)

 

int main(void) {
uint8_t mask=0xf5,shifter=0;int8_t i;

for (i=7;i>=0;i--)
{
    if (mask&(1<<i))
    {
        shifter|=1;
        shifter>>=1;
    }
}

    printf("0x%.2x",shifter);

    return 0;
}

 

sparrow2 wrote:

 

If it work's I guess it's ok but you overcomplicate the problem, (again single step the code).

 

But what is good code

fast

small

easy to read

 

and yours are none of the above (sorry to say).

OK, for my sketch, how would develop a code which has the good code features?

 

This is the best I could get until now:

#include <avr/io.h>
#include <util/delay.h>
#include <inttypes.h>
#include "I2C_m.h"
#include "sensors.h"

void DH11_init(void)
{
  _delay_ms(2000);										// 2s delay for DH11 to response
  TCCR0B = 0x02;										// enable timer0 clkT2S/8 and divide reading /2
}												// to get 1us resolution				

void DH11_read(uint8_t *bits_timing)
{
	uint8_t TMR_TEMP=0,DECODE_TEMP=0;int8_t j,i;
	DDRB = 0xFF;										// Pin0 output
	PORTB &= ~(1<<PB0);									// pull PB0 LOW	for 18ms
	_delay_ms(18);
	PORTB |= (1<<PB0);									// pull it HIGH again
	DDRB = 0xFE;

	while((PINB & (1<<PINB0)));							        // leading HIGH pulse waiting for response
	while(!(PINB & (1<<PINB0)));						                // 1st response 80us LOW
	while((PINB & (1<<PINB0)));							        // 2nd response 80us HIGH

	for (i=0;i<5;i++)									// 5 bytes for 40-bits DH11 data
    {	uint8_t DH11_code =0;							                // local vars
		for (j=7;j>=0;j--)								// 8-bits loop
		{
			while(!(PINB & (1<<PINB0)));                                            // wait for 0 leading bit
			TCNT0=0;								// clear the timer0
			while(PINB & (1<<PINB0));                                               // high bit 0 or 1
			TMR_TEMP = TMR0_MSK;						        // read timer0 mask
			if (TMR_TEMP < 35 && TMR_TEMP > 20)			                // 0 bit length is 26-28us
				DH11_code |=1;							// set 0-bit
			DH11_code>>=1;								// move the shifter to the right
		}
		bits_timing[i] = DH11_code;						        // copy the result to the array
    }
}

 

 

 

if your goal is just small (you don't need speed, you wait most of the time anyway, so in real life you should have this code placed in an interrupt)

I agree with interrupt implementation. But where do I wait?

 

 

for smaller code try :

declare i and j inside the for loops to make it 100% clear for the compiler that they are only local. 

Yes, I know this has a relationship with the function overhead and the code overall, but at the moment of function execution time, the function overhead adds to the program total overhead, so declaring the variables inside the function helps in real time operation if multiple functions work in different times.

 

instead of shift try *2 or /2 depending of direction (because calculations in C has to be of int shifts sometimes are on 16 bit even for a 8 bit variable it don't seem to be a problem for * and / and it will still just make a shift).

you can get rid of the inner for loop(j) and make a while on shift (when done shift become 0 (false) ).

Couldn't understand this part because I think it's about advantages of / and * over shifts, and how to do the shifts with / and * but I don't know about these skills.

 

 

if you only use the function once make it inline (then you save the function call)

What I know about inline functions, that they are the most frequent & small functions that I may apply in the program. To get faster call of a certain function, but it has to be relatively small, define it as "inline".

 

What I didn't understand about this part is:

1. Why would I define the function as "inline" if it has less use in the program?

2. How to save the function call? 

 

 

and if you can don't pass a pointer but use a global array.

Why? Isn't a pointer better than arrays?

 

 

But I guess we are down to less than max 20-50 bytes you can save unless you write it in ASM but that's an other day.

I didn't try any ASM codes yet :)

 

Last Edited: Thu. Aug 23, 2018 - 05:18 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
int main(void) {
uint8_t mask=0xf5,shifter=0;int8_t i;

for (i=7;i>=0;i--)
{
     shifter<<=1;
    if (mask&(1<<i))
    {
        shifter|=1;

    }

}

    printf("0x%.2x",shifter);

    return 0;
}

You didn't think about this too carefully did you? No pencil and paper?? And you wonder why things don't work for you. 

 

You haven't also taken on board the various comments regarding code clarity - no if{}. You also violate the defacto practice of putting constants in upper case - somehow normal variables become upper case in your code.

 

The secret to writing good comments in your code is not to state the obvious. The comments are to describe the intent of the code, they should tell a story. I've given you some good hints and references in the past that you've clearly ignored.

 

 

 

 

Last Edited: Thu. Aug 23, 2018 - 05:37 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

ISR "But where do I wait?" you don't you trigger on the edges of the port pin, and most chips has a timer function for just what you want (only count when a port pin is either high or low )

 

for an uint  /2 is the the same as shift right (same as in decimal /10) and *2 is a left shift.

 

pointer array:

if you can live with a static array, you don't need to pass anything. And you can still use a pointer inside the function.

 

inline:

if a function only is used once the compiler can save the call by placeing the function where it's used if it's inline.

 

I did not compile your code but it looks like a ASM version take about 40 instructions (80 byte), (perhaps a tad more if some of the IO's is outside the IN/OUT range.)

so even if C use more it's still a small rutine in the hole picture.  

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

What???

 

			if (TMR_TEMP < 35 && TMR_TEMP > 20)			                // 0 bit length is 26-28us
				DH11_code |=1;							// set 0-bit
			DH11_code>>=1;

You realize you are setting bit 0 then immediately annihilating it with the shift...

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

sparrow2 wrote:
inline:

if a function only is used once (sic) the compiler can save the call by placeing the function where it's used if it's inline.

The number of times the function is used is irrelevant:  'inline' means that the code should always be placed in line - not called.

 

Clearly, for a function that's called a lot, that verbatim repetition of the same code  can cause "bloat".

 

As ever, it's a tradeoff.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

wolfrose wrote:
When I started learning C, I learned that if it's only one instruction after a for or while, then there's no need for the curly braces

True, there's no need - but most professionals will recommend that you use them anyhow - they (help to) prevent several common errors ...

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

#49

The number of times the function is used is irrelevant:  'inline' means that the code should always be placed in line - not called. 

try to be a bit serious, OP want to make the code small, so unless the function only is called once it should be obvious (for all other than you) that the code will be bigger if the code is called from different places.

unless it's a relative small function with many parameters the code actually could be smaller.(but not the case here)

And unless the function is static the compiler don't know how many time the function is called and will only make it inline if a function is small (not the case here).  

Pages