Little confusion on arrays ( Mikroc )

Discussion in 'Programmer's Corner' started by LETITROLL, May 1, 2014.

  1. LETITROLL

    Thread Starter Member

    Oct 9, 2013
    218
    2
    Hi all of you :) .


    I have just finished writing a program and it works but as you see in the code , i have used portb= Leds; into the while(1) loop , why i can't work with it outside the loop ? tricky logic here :confused:

    My code :

    Code ( (Unknown Language)):
    1.  
    2. [B]
    3. #define start porta.b0
    4. #define led1  portb.b0
    5. #define led2  portb.b1
    6. #define led3  portb.b2
    7. #define led4  portb.b3
    8.  
    9. int Leds[6]= {0x00,0x01,0x02,0x04,0x08};
    10. int i=0 ;
    11.  
    12. void main() {
    13.  
    14. porta=0x00;
    15. portb=0x00;
    16. trisa=0xff;
    17. trisb=0x00;
    18. i=0;
    19.  
    20. while(1) {
    21. if( start==1){
    22. delay_ms(200);
    23. i++;
    24. portb= Leds[i];
    25. delay_ms(500);
    26. }
    27. if(i==4){
    28. i=0;
    29. }
    30. }
    31. }
    32. [/i][/B][i]
    33.  
    34. [/i]
     
    Last edited: May 1, 2014
  2. WBahn

    Moderator

    Mar 31, 2012
    17,743
    4,795
    (1) It would be nice if you would format your code so that it is easily readable.

    (2) What do you mean by using portb= Leds; outside the loop? You are using the variable 'i' and the value is changing each pass through the loop. What is it that you are trying to achieve that you think would be reasonably done with putting that statement outside the loop?
     
  3. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,387
    1,605
    Happy to see your progress in little more then a half hour. Also glad you use code tags!

    However, I do not understand your question. You define Leds[] as a global variable, you should be able to use it anywhere in your code.

    (Aside: LEDs is 5 elements in size and numbered from 0 to 4, or as some would say "five is right out!")

    And welcome to the forums!
     
    Last edited: May 1, 2014
  4. LETITROLL

    Thread Starter Member

    Oct 9, 2013
    218
    2


    I see your point , the variable is always changing so it should be always inside the loop .
     
    Last edited: May 1, 2014
  5. LETITROLL

    Thread Starter Member

    Oct 9, 2013
    218
    2
    0 is when the leds are all off , 1,2,3,4 are for the leds and 6 is for the array ending as far as i understand .

    I guess am starting to grasp C# a little bit .
     
  6. WBahn

    Moderator

    Mar 31, 2012
    17,743
    4,795
    Please don't go back and make substantive changes to earlier posts, particularly if others have responded about them. It makes following a thread very difficult. Instead, make a follow-up post or at least indicate what edits have been made.

    I don't understand some of your new code. And it would still be nice if you formatted it. Here is an example of what I mean:

    Code ( (Unknown Language)):
    1.  
    2. [I]  // I have no idea what this does or is supposed to do
    3. #define start porta.b0
    4. #define led1  portb.b0
    5. #define led2  portb.b1
    6. #define led3  portb.b2
    7. #define led4  portb.b3
    8.  
    9. int Leds[6]= {0x00,0x01,0x02,0x04,0x08}; // You only have five elements
    10. int i=0 ;
    11.  
    12. void main() {
    13.  
    14.    porta=0x00;
    15.    portb=0x00;
    16.    trisa=0xff;
    17.    trisb=0x00;
    18.    i=0;
    19.  
    20.    while(1) {
    21.       if( start==1) {
    22.          delay_ms(200);
    23.          i++;
    24.          portb= Leds; // This makes no sense.
    25.          delay_ms(500);
    26.       }
    27.       if(i==4) {
    28.          i=0;
    29.       }
    30.    }
    31. }
    32.  
    33. [/I]


    Hopefully you will agree that this is much more readable and easy to follow.

    It's hard to know if this code has a chance of accomplishing what you want since you give no indication of what it is you want the code to accomplish.

    The code above should give you lots of grief.

    First, you are assigning the pointer to an array to your port when you say "portb = Leds;". Assuming that it compiles and runs, it almost certainly is not doing what you want.

    Second, even if it is doing what you want, you are doing the exact same thing each time through the loop. So you will see no change at all, which almost certainly defeats the purpose.

    Third, as soon as port.b0 is 0 your code will become unresponsive (or at least appear that way) because the if() test will fail from that point on since you have no means outside of the if() block of affecting the contents of portb.

    Fourth, you initialize portb to 0 before the while() loop. Doesn't that set portb.b0 to 0? So won't it fail the if() test right from the beginning?

    Finally, some style suggestions (and these are certainly only suggestions):

    1) Align your opening and closing curly braces. This makes it trivial to spot problems with code segments that are not properly enclosed or nested.

    2) Whenever you compare a variable to a constant, especially for equality, put the constant on the left side. Looks a bit awkward at first, but saying something like "if (a=5)" is perfectly valid syntax but almost certainly a logic error. It's an easy mistake to make. But saying "if (5=a)" is a syntax error so the compiler can flag it and you can change it to "if (5==a)".

    3) Try to make your traps all inclusive, so instead of checking for i being exactly equal to 4, check for it being greater than or equal to 4.

    So something more like this:

    Code ( (Unknown Language)):
    1.  
    2. #define start porta.b0
    3. #define led1  portb.b0
    4. #define led2  portb.b1
    5. #define led3  portb.b2
    6. #define led4  portb.b3
    7.  
    8. int Leds[]= {0x00,0x01,0x02,0x04,0x08}; // Let the compiler count the elements.
    9. int i=0 ;
    10.  
    11. void main() {
    12.  
    13.    porta=0x00;
    14.    portb=0x00;
    15.    trisa=0xff;
    16.    trisb=0x00;
    17.  
    18.    i=0;
    19.    while (1)
    20.    {
    21.       portb = Leds[i++];
    22.       delay_ms(500);
    23.       if (i >= 5)
    24.          i=0;
    25.    }
    26. }
    27.  
    28.  
    I don't know if this will do what you want because, again, you haven't told us what you want. But hopefully it is a pointer in the right direction.
     
  7. LETITROLL

    Thread Starter Member

    Oct 9, 2013
    218
    2
    Sorry for the confusion .

    Just like i mentioned on the first post , i have managed to get the program to work like i need ; And basically the purpose of the code is changing the variable i witch then changes the selection of array numbers from 0 to 6 , so for example when you press a button you have i++ , and when you arrive to value 1 (0x01) portb changes to that value according to portb= Leds; , in this case the led connected to pic's portb.b0 turns on .
     
  8. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,387
    1,605
    aside to LETITROLL: all the codeheads here understood the intent of your program when first posted (even in the other thread).

    To expand on something WBahn said it is far preferable to let the compiler figure out how big your array is using:
    Code ( (Unknown Language)):
    1.    int Leds[]= {0x00,0x01,0x02,0x04,0x08}; // Let the compiler count the elements.
    2.  
    I would like to add to this. The size of the array is used as a comparison in the line:
    Code ( (Unknown Language)):
    1.    if (i >= 5)
    2.  
    While that may be correct it uses something called a "magic number." What is that 5? Where did it come from? How is it computed?
    Worse, what happens if you add a few more states to the array but forget to change 5?
    Here is a lovely fix for all that:
    Code ( (Unknown Language)):
    1.    if (i >= sizeof(Leds)/sizeof(*Leds))
    2.  
    That does a ton of work for you. It automatically figures out how big the array is if your code changes, and it defines what the comparison is computed. And perhaps the best part, your compiler in the PC does the computation once, and the resulting number (here it still is just 5) goes into yout PIC code.


    Generally, that is called "compile time" verses "run time." Do any work you can at compile time so it is free at run time.

    The keyword "sizeof" computes the number of storage elements for a given variable. The divisor "sizeof(*Leds)" isn't necessary here but will be necessary on any type of variable above a char. Plus this is a compile time computation so we really don't care it takes longer anyway.
     
  9. tshuck

    Well-Known Member

    Oct 18, 2012
    3,531
    675
    You are not using C#. You are using C, and mikroC to be specific.

    As WBahn said, don't go modifying your initial post once people have responded, it makes the thread nearly impossible to follow. I was going to respond much earlier, but I didn't know what others were referring to and what edits you had made since then.
     
    ErnieM likes this.
  10. WBahn

    Moderator

    Mar 31, 2012
    17,743
    4,795


    Ah, I see part of my confusion. My vision is blurry enough that I couldn't see that your "start" was on porta. I thought it was on portb as well (and hence the same as led0).

    You still seem confused on the array bounds. You have FIVE array elements, so the array index can only have values from 0 through 4 (inclusive).

    Code ( (Unknown Language)):
    1.  
    2.  
    3. // PORT A bit assignments
    4. #define start porta.b0
    5.  
    6. // PORT B bit assignments
    7. #define led1  portb.b0
    8. #define led2  portb.b1
    9. #define led3  portb.b2
    10. #define led4  portb.b3
    11.  
    12. int Leds[]= {0x00,0x01,0x02,0x04,0x08};
    13. int Leds_dim = sizeof(Leds)/sizeof(*Leds);
    14. int i=0 ;
    15.  
    16. void main(void)
    17. {
    18.    porta = 0x00;
    19.    portb = 0x00;
    20.    trisa = 0xFF;
    21.    trisb = 0x00;
    22.  
    23.    i=0;
    24.    while(1)
    25.    {
    26.       if( 1 == start )
    27.       {
    28.          delay_ms(200);      // Small delay after detecting button press
    29.          portb= Leds[i++];
    30.          delay_ms(500);      // Remainder of delay to allow for button release
    31.          if( i >= Leds_dim ) // Cycle back to start of pattern
    32.             i=0;
    33.       }
    34.    }
    35. }
    36.  
    Notice that I called the one variable Leds_dim and not something like i_max. If I had called it i_max, then this would have suggested that it represented the maximum value that i could take on, but this is ambiguous because is that the max value it could take on as an index, or the max value that it can take on after incrementing it following it being used to access the last legal index? By calling it Leds_dim, that indicates that it is simply the dimension of the Leds array, something whose meaning is unambiguous.

    Another way to approach this, which would save you the space needed for your array, is to bit bang it.

    Code ( (Unknown Language)):
    1.  
    2.  
    3. // PORT A bit assignments
    4. #define start porta.b0
    5.  
    6. // PORT B bit assignments
    7. #define led1  portb.b0
    8. #define led2  portb.b1
    9. #define led3  portb.b2
    10. #define led4  portb.b3
    11.  
    12. unsigned char pattern;
    13.  
    14. void main(void)
    15. {
    16.    porta = 0x00;
    17.    portb = 0x00;
    18.    trisa = 0xFF;
    19.    trisb = 0x00;
    20.  
    21.    pattern=0;
    22.    while(1)
    23.    {
    24.       if( 1 == start )
    25.       {
    26.          delay_ms(200);       // Small delay after detecting button press
    27.          portb = pattern;
    28.          pattern = ((pattern)? (pattern<<1) : (1))&0xFF; // Advance pattern
    29.          delay_ms(500);       // Remainder of delay to allow for button release
    30.       }
    31.    }
    32. }
    33.  
     
  11. WBahn

    Moderator

    Mar 31, 2012
    17,743
    4,795
    I understood what he probably wanted, but the point I'm trying to get across is that don't rely on people being able to guess what you are trying to do. Especially if they have to guess based on reading code that you have questions about!

    I absolutely agree. I almost put that in but didn't know if that wouldn't be adding too much new complexity at one time so left it for a follow up.
     
  12. LETITROLL

    Thread Starter Member

    Oct 9, 2013
    218
    2
    Ok i appreciate your special inputs .
    So from your experiences in programming , any advices to get really good at c programming
    for embedded systems . Also a nice book would be really helpful .
     
  13. WBahn

    Moderator

    Mar 31, 2012
    17,743
    4,795
    The best way to get really good at any programming is to write lots of programs. That's not a flippant answer -- I'm very serious.

    But not just any program -- write programs that are challenging in some way. They don't have to be big or long, either. And play with things. See if you can find two or three significantly different ways to solve the same little problem.

    While not intended for embedded systems, you might look at the Project Euler website. There are tons (literally hundreds) of challenge problems for people to code up and submit. Many of them only take a few minutes (others could take weeks). That's a good way to build your skillset with C in general.

    Something else that you might find useful and that is a bit (not pun intended) more relevant to embedded programming is to start with the standard functions that get just one character from the input and place just one character to the output and then YOU write all of the other common I/O and string functions (or most of them) on top of them, including a simplified version of printf(). That will force you to really learn how data is represented and manipulated and also give you an intuitive awareness of the complexity of many of these operations and why, for example, printf() is such an expensive function in an embedded application.
     
    LETITROLL likes this.
  14. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,387
    1,605
    I think everyone who writes C should have a copy of this book: The C Programming Language. Oft called K&R after the authors, the "R" (Dennis Ritchie) orgionally designed C: not a compiler, the language itself.

    There is a lot of very good and very bad info on the web. Do try to find a style guide as the best way to make code readable is white space.

    Write lots of code helps. Don't write "obfuscated" C (unless entering s contest). Make your statements plain, use parenthesis wherever even possibly necessary, and always comment your code.


     
    LETITROLL likes this.
  15. hexreader

    Active Member

    Apr 16, 2011
    250
    82
    Try turning on case sensitivity for the compiler.

    You will find many errors in your code that the compiler has hidden from you.

    A PIC does not have a register with the name porta, but it may well have a register with the name PORTA

    Might seem pedantic, but if you are to learn coding style, then you might as well learn good practice.

    If you ever port code, or have reason to make the compiler case sensitive, then getting this right from the start may save grief in the future.
     
    Last edited: May 3, 2014
    LETITROLL likes this.
Loading...