Learning C

[crutschow]

The language itself has few instruction, maybe as few as one dozen

According to this reference, there are 32 keywords.

 

[augustinetez]

And umpteen operators not included in that list.

 

[Pommie]

That list of 32 includes the variable types (E.G. int, char etc.) and keywords that shouldn't be used (E.G. goto). When you take out the variable types then it's less than 2 dozen - not as few as I thought but not a lot to learn. It can be obscure but most things can be done in a simpler way and should be so as to be understandable.

The umpteen operators I'll concede.

Mike.

 

[Ian Rogers]

And umpteen operators not included in that list.

Wait until you start C++.... You "think" you have seen operators...

 

[augustinetez]

I'm having a hard enough time with C, not going anywhere near C++.

 

[nsaspook]

That list of 32 includes the variable types (E.G. int, char etc.) and keywords that shouldn't be used (E.G. goto). When you take out the variable types then it's less than 2 dozen - not as few as I thought but not a lot to learn. It can be obscure but most things can be done in a simpler way and should be so as to be understandable.

The umpteen operators I'll concede.

Mike.

I disagree on goto, it should be used as a proper structured concept, not as a pasta twister.
**broken link removed**

http://www.cprogramming.com/tutorial/goto.html

http://embeddedgurus.com/barr-code/2018/06/cs-goto-keyword-should-we-use-it-or-lose-it/

http://koblents.com/Ches/Links/Month-Mar-2013/20-Using-Goto-in-Linux-Kernel-Code/

On Sun, 12 Jan 2003, [redacted by request 12/3/2020] wrote:
>
> However, I have always been taught, and have always believed that
> "goto"s are inherently evil. They are the creators of spaghetti code

No, you've been brainwashed by CS people who thought that Niklaus Wirth
actually knew what he was talking about. He didn't. He doesn't have a
frigging clue.

> (you start reading through the code to understand it (months or years
> after its written), and suddenly you jump to somewhere totally
> unrelated, and then jump somewhere else backwards, and it all gets ugly
> quickly). This makes later debugging of code total hell.

Any if-statement is a goto. As are all structured loops.

Ans sometimes structure is good. When it's good, you should use it.

And sometimes structure is _bad_, and gets into the way, and using a
"goto" is just much clearer.

For example, it is quite common to have conditionals THAT DO NOT NEST.

In which case you have two possibilities

- use goto, and be happy, since it doesn't enforce nesting

This makes the code _more_ readable, since the code just does what
the algorithm says it should do.

- duplicate the code, and rewrite it in a nesting form so that you can
use the structured jumps.

This often makes the code much LESS readable, harder to maintain,
and bigger.

The Pascal language is a prime example of the latter problem. Because it
doesn't have a "break" statement, loops in (traditional) Pascal end up
often looking like total sh!t, because you have to add totally arbitrary
logic to say "I'm done now".

Linus

 

[crutschow]

My final thoughts on C.
April Fool.

 

[augustinetez]

In that case just do
encoder_count = table[(previous & 0x0f)];
Note the missing +.
Your mainloop will need to check encoder_count and use it if it's not zero and then set it to zero.
Also, define it as a signed char.
char encoder_count;
or
int8_t encoder_temp;

Mike.

OK, dumb question time again - char encoder_count; - why char and not 'int' or something else? (bearing in mind encoder_count will hold 0, +1 or -1.

In some of the 'stuff' I've been reading, several authors seem to explicitly state that 'char' is for, well, characters as in letters - a,b,c etc - for putting up on an LCD or some such display device.

I'm working towards getting one LED to light if you turn the encoder one way and a different LED if you turn it the other - I think I've got all the bits of code written/modified as needed and will string it all together later and see where I've stuffed it up.

 

[Pommie]

Char is a single byte variable. On a 32 or 64 bit PC then it's much quicker to use an int but on a micro controller it's better to use char or as I prefer int8_t or better still, unsigned char or uint8_t.

Mike.
Edit, in this case it has to be signed but where it makes no difference it's quicker to use unsigned.

 

[augustinetez]

OK, I've got to this point with a lot of hair loss so far, it compiled (apparently) but I haven't run it on a chip yet and it generated the warnings show after the code so I obviously haven't got to grips with calling functions properly yet The EEprom bit at the bottom is from a post further back and can be ignored at this stage.

#include <xc.h>
#include <stdint.h>

// CONFIG1
#pragma config FOSC = INTOSC    // Oscillator Selection->INTOSC oscillator: I/O function on CLKIN pin
#pragma config WDTE = SWDTEN    // Watchdog Timer Enable->WDT controlled by the SWDTEN bit in the WDTCON register
#pragma config PWRTE = OFF      // Power-up Timer Enable->PWRT disabled
#pragma config MCLRE = ON       // MCLR Pin Function Select->MCLR/VPP pin function is MCLR
#pragma config CP = OFF         // Flash Program Memory Code Protection->Program memory code protection is disabled
#pragma config CPD = OFF        // Data Memory Code Protection->Data memory code protection is disabled
#pragma config BOREN = ON       // Brown-out Reset Enable->Brown-out Reset enabled
#pragma config CLKOUTEN = OFF   // Clock Out Enable->CLKOUT function is disabled. I/O or oscillator function on the CLKOUT pin
#pragma config IESO = ON        // Internal/External Switchover->Internal/External Switchover mode is enabled
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enable->Fail-Safe Clock Monitor is enabled

// CONFIG2
#pragma config WRT = OFF        // Flash Memory Self-Write Protection->Write protection off
#pragma config PLLEN = ON       // PLL Enable->4x PLL enabled
#pragma config STVREN = ON      // Stack Overflow/Underflow Reset Enable->Stack Overflow or Underflow will cause a Reset
#pragma config BORV = LO        // Brown-out Reset Voltage Selection->Brown-out Reset Voltage (Vbor), low trip point selected.
#pragma config LVP = OFF        // Low-Voltage Programming Enable->High-voltage on MCLR/VPP must be used for programming

#define AD9850        // Using the AD9850
//#define AD9851        // Using the AD9851

#ifdef AD9850
//    For 125 MHz Oscillator =======
#define ref_osc  0x225C17D0L
#endif
#ifdef AD9851
//    For 180 MHz (30 MHz clock and 6x multiplier)                             
#define ref_osc 0x17DC65DEL
#endif

//Limit contains the upper limit frequency as a 32 bit integer.
#define limit 0x0053EC60L

//Low_limit contains the lower frequency limit as a 32 bit integer
#define limit_low 0x004C4B40L

//    Default contains the default startup frequency as a 32 bit integer.
//    This will be overwritten by the frequency save routine in normal use.
#define default 0x004C4B40L

//    Frequency at which Calibration will take place
#define cal_freq 0x00501BD0L

#define DDSdata AN0
#define DDSclock AN2
#define DDSload AN1
#define _XTAL_FREQ (32000000)

const signed char table[] = {0,-1,+1,0,+1,0,0,-1,-1,0,0,+1,0,+1,-1,0};
void sendDDSword(int);
void writeEEPROM(unsigned char,unsigned char);
volatile signed int encoder_count;

void main(void){
    PORTA=0;
    TRISA=0b00111100;
    ANSELA=0;   
    // SCS FOSC; SPLLEN disabled; IRCF 8MHz_HF;
    OSCCON = 0x70;      //set osc to 32MHz
    BORCON = 0x00;
    while(PLLR == 0);
    // WDTPS 1:65536; SWDTEN OFF;
    WDTCON = 0x16;
    WPUA = 0x00;
    OPTION_REGbits.nWPUEN = 1;
  //  writeEEPROM(0,0x23);
    while(1){
       encoder_count = 0; 
      
       encoder_count = encoder_click(encoder_count);
      
         //attach led to LATA0
    if (encoder_count == 1){
        LATAbits.LATA0 = 1;
        __delay_ms(100);
        LATAbits.LATA0 = 0;
        encoder_count = 0;
        return;
    }
    //attach led to LATA1
    if (encoder_count == -1){
        LATAbits.LATA1 = 1;
        __delay_ms(100);
        LATAbits.LATA1 = 0;
        encoder_count = 0;
        return;
    }
    }
}

int encoder_click(int encoder_count)
  {
  static unsigned char previous = 0;
  unsigned char temp;

  temp = 5;

  while(temp--){ /* debounce */
    ;
    }

  temp = PORTA;     /* Read port - encoder on bits 3 & 4*/
  temp >>= 3;       /* Shift input to bit positions 1, 0 */
  temp &= 0x03;     /* Mask out bits */
  previous <<= 2;   /* shift the previous data left two places */
  previous |= temp; /* OR in the two new bits */

  encoder_count = table[(previous & 0x0f)];  /* Index into table */
  return encoder_count;
  }








/*
void writeEEPROM(unsigned char address,unsigned char data){
    EEADRL = address;
    EEDATL=data;
    CFGS=0;
    EEPGD=0;
    WREN=1;
    EECON2=0x55;
    EECON2=0xAA;
    WR=1;
    WREN=0;
    while(WR==1);
}*/

And the warning messages are:

newmain.c:74:24: warning: implicit declaration of function 'encoder_click' is invalid in C99 [-Wimplicit-function-declaration]
       encoder_count = encoder_click(encoder_count);
                       ^
1 warning generated.
"C:\Program Files\Microchip\xc8\v2.32\bin\xc8-cc.exe"  -mcpu=12F1840 -Wl,-Map=dist/default/production/Simple_VFO.X.production.map  -DXPRJ_default=default  -Wl,--defsym=__MPLAB_BUILD=1   -mdfp="C:/Program Files/Microchip/MPLABX/v5.45/packs/Microchip/PIC12-16F1xxx_DFP/1.2.63/xc8"  -fno-short-double -fno-short-float -O0 -fasmfile -maddrqual=ignore -xassembler-with-cpp -mwarn=-3 -Wa,-a -msummary=-psect,-class,+mem,-hex,-file  -ginhx032 -Wl,--data-init -mno-keep-startup -mno-osccal -mno-resetbits -mno-save-resetbits -mno-download -mno-stackcall -std=c99 -gdwarf-3 -mstack=compiled:auto:auto      -Wl,--memorysummary,dist/default/production/memoryfile.xml -o dist/default/production/Simple_VFO.X.production.elf  build/default/production/newmain.p1     
newmain.c:74:: warning: (1518) direct function call made with an incomplete prototype (encoder_click)

 

[Pommie]

You need a prototype for encoder_click, add,

Int encoder_click (int);

to the top of the code.

Actually, passing encoder_count is unnecessary, I'm away for the (long)
Weekend but will have another look on Tuesday.


Mike.

 

[augustinetez]

Thanks Mike.

Compiled without any warnings this time.

I'll burn it in to a chip tomorrow and see what happens.

 

[augustinetez]

I've trimmed up the code and been playing about with various things to see what happens (hence the commented out bits of code), but either my understanding of what this should do against what it is actually doing is way off or I'm going completely nuts.

#include <xc.h>
#include <stdint.h>

// CONFIG1
#pragma config FOSC = INTOSC    // Oscillator Selection->INTOSC oscillator: I/O function on CLKIN pin
#pragma config WDTE = SWDTEN    // Watchdog Timer Enable->WDT controlled by the SWDTEN bit in the WDTCON register
#pragma config PWRTE = OFF      // Power-up Timer Enable->PWRT disabled
#pragma config MCLRE = ON       // MCLR Pin Function Select->MCLR/VPP pin function is MCLR
#pragma config CP = OFF         // Flash Program Memory Code Protection->Program memory code protection is disabled
#pragma config CPD = OFF        // Data Memory Code Protection->Data memory code protection is disabled
#pragma config BOREN = ON       // Brown-out Reset Enable->Brown-out Reset enabled
#pragma config CLKOUTEN = OFF   // Clock Out Enable->CLKOUT function is disabled. I/O or oscillator function on the CLKOUT pin
#pragma config IESO = ON        // Internal/External Switchover->Internal/External Switchover mode is enabled
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enable->Fail-Safe Clock Monitor is enabled

// CONFIG2
#pragma config WRT = OFF        // Flash Memory Self-Write Protection->Write protection off
#pragma config PLLEN = ON       // PLL Enable->4x PLL enabled
#pragma config STVREN = ON      // Stack Overflow/Underflow Reset Enable->Stack Overflow or Underflow will cause a Reset
#pragma config BORV = LO        // Brown-out Reset Voltage Selection->Brown-out Reset Voltage (Vbor), low trip point selected.
#pragma config LVP = OFF        // Low-Voltage Programming Enable->High-voltage on MCLR/VPP must be used for programming

#define DDSdata LATA0 //AN0
#define DDSload LATA1 //AN1
#define DDSclock LATA2 //AN2
#define _XTAL_FREQ (32000000)

char encoder_click (char);
//volatile signed char encoder_count;
char encoder_count = 0;
unsigned char temp;
static unsigned char previous = 0;
char dir = 0;
//const signed char table[] = {0,-1,1,0,1,0,0,-1,-1,0,0,1,0,1,-1,0};
const char table[] = {0,0xff,1,0,1,0,0,0xff,0xff,0,0,1,0,1,0xff,0};

void main(void){
    PORTA=0;
    TRISA=0b00111000;
    ANSELA=0;  
    // SCS FOSC; SPLLEN disabled; IRCF 8MHz_HF;
    OSCCON = 0x70;      // set osc to 32MHz
    BORCON = 0x00;
    while(PLLR == 0);
    // WDTPS 1:65536; SWDTEN OFF;
    WDTCON = 0x16;
    WPUA = 0x00;
    OPTION_REGbits.nWPUEN = 1;
    // previous = 0;
    // encoder_count = 0;
   
    while(1){
           
       dir = encoder_click(encoder_count);
     
        // attach led to LATA0
    if (dir == 1){
        //LATAbits.LATA0 = 1; // DDSdata
        DDSdata = 1; // LATA0
        __delay_ms(100);
        //LATAbits.LATA0 = 0;
        DDSdata = 0; // LATA0
        //encoder_count = 0;
        return;
    }
       //attach led to LATA1
    if (dir == 0){
        //LATAbits.LATA1 = 1; // DDSload
        DDSload = 1; // LATA1
        __delay_ms(100);
        //LATAbits.LATA1 = 0;
        DDSload = 0; // LATA1
        //encoder_count = 0;
        return;
    }
    //attach led to LATA2
    if (dir == 0xff){
        //LATAbits.LATA2 = 1; // DDSclock
        DDSclock = 1; // LATA2
        __delay_ms(100);
        //LATAbits.LATA2 = 0;
        DDSclock = 0; // LATA2
        //encoder_count = 0;
        return;
    }
    }
}

char encoder_click(char encoder_count)
  {
  temp = PORTA;     /* Read port - encoder on bits 3 & 4*/
  temp >>= 3;       /* Shift input to bit positions 1, 0 */
  temp &= 0x03;     /* Mask out bits */
  previous <<= 2;   /* shift the previous data left two places */
  previous |= temp; /* OR in the two new bits */

  //encoder_count = table[(previous & 0x0f)];  /* Index into table */
  encoder_count = table[previous];  /* Index into table */
  return encoder_count;
  }

What I think it should do:

1/ All the set-up.

2/ In to main

3/ Jump out to encoder_click

4/ Check if the encoder has moved

5/ If no movement the variable 'encoder_count' should = 0, if it moves one way it should = 1 and if it moves the other way it should = 0xff

6/ Return to main

7/ Take the returned value from encoder_count, place that in 'dir' and then light the approriate LED viz light the LED on RA0 for movement one way, light the LED on RA1 for no movement (or illegal step) or light the LED on RA2 for movement in the opposite direction

8/ Do it all again from 3/

What it actually does:

All from 1-4 above = OK

5 returns the wrong values

6 = OK

7 Lights the wrong LED in two situations (explained below)

8 = OK

LED's: - On first power up and the encoder not moved - LED on RA0 lights indicating returned value in 'encoder_count' is 1 instead of 0.

Turning the encoder in one direction, this time it will make the LED on RA0 blink, indicating 'encoder_count' is returning the value 1.

Turning the encoder in the opposite direction, the LED on RA1 blinks, indicating 'encoder_count' is returning the value 0 instead of 0xFF.

When the encoder stops turning, the LED on RA0 again lights, indicating' encoder_count' is returning the value of 1 instead of 0.

The encoder is one of those cheap detented things with 24 detents and it's rest state is both A & B outputs open, so with the pullups of the circuit, returns RA3 = 1 & RA4 = 1

Messing with the value that the variable 'previous' is preset with (0 or 0x03), changes which two LED's actually light, blink, do something - the above is with 'previous' set to 0.

My calculator says that, on start up with 'previous' = 0 and no encoder movement - ORing in the value of 'temp' (0x03) should return a value in 'previous' of 0x03 and that should pull the value of 0 from the table and put it in 'encoder_count'.

Then the next time through this code, again without the encoder moving, 'previous' is left shifted twice and should hold the value of 0x0C, 'temp' still 0x03, ORed in with 'previous' should be 0x0F which again should pull the value of 0 from the table to be put in 'encoder_count'.

And yes, the eventual aim is to stay within the encoder_click function until the encoder actually moves to mimic what happens in my asm file (as a reminder, I am trying to convert an existing project from asm to C as part of learning C).

I've also attached the schematic of how my test bed is set up at the moment.

 

[augustinetez]

And just to show I'm not sitting around waiting for answers - I found the problem of it not doing what was expected (and I haven't edited the problem out in the previous post so other beginners might learn something from it - did take me two days to find it, but that's what getting old does for ya' ).

I was looking in the wrong place (code wise), it was in the Config and not within 'main' - MCLRE was ON when it should have been OFF.

 

[augustinetez]

And a further update, I have (as far as I can tell) the encoder routine now constantly polling itself until the encoder moves one way or the other.

Either need a better encoder routine or to find some way of annihilating the bounce 'spikes' when it pops out of the routine and turns on the opposite direction LED

Also need to account for the 4 pulses in each detent to detent step so only one step per detent is output (hope that made sense).

#include <xc.h>
#include <stdint.h>

// CONFIG1
#pragma config FOSC = INTOSC    // Oscillator Selection->INTOSC oscillator: I/O function on CLKIN pin
#pragma config WDTE = SWDTEN    // Watchdog Timer Enable->WDT controlled by the SWDTEN bit in the WDTCON register
#pragma config PWRTE = ON       // Power-up Timer Enable->PWRT enabled
#pragma config MCLRE = OFF      // MCLR Pin Function Select->MCLR/VPP pin function is INPUT
#pragma config CP = OFF         // Flash Program Memory Code Protection->Program memory code protection is disabled
#pragma config CPD = OFF        // Data Memory Code Protection->Data memory code protection is disabled
#pragma config BOREN = OFF      // Brown-out Reset Enable->Brown-out Reset disabled
#pragma config CLKOUTEN = OFF   // Clock Out Enable->CLKOUT function is disabled. I/O or oscillator function on the CLKOUT pin
#pragma config IESO = ON        // Internal/External Switchover->Internal/External Switchover mode is enabled
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enable->Fail-Safe Clock Monitor is enabled

// CONFIG2
#pragma config WRT = OFF        // Flash Memory Self-Write Protection->Write protection off
#pragma config PLLEN = ON       // PLL Enable->4x PLL enabled
#pragma config STVREN = ON      // Stack Overflow/Underflow Reset Enable->Stack Overflow or Underflow will cause a Reset
#pragma config BORV = LO        // Brown-out Reset Voltage Selection->Brown-out Reset Voltage (Vbor), low trip point selected.
#pragma config LVP = OFF        // Low-Voltage Programming Enable->High-voltage on MCLR/VPP must be used for programming

#define DDSdata LATA0 //AN0
#define DDSload LATA1 //AN1
#define DDSclock LATA2 //AN2
#define _XTAL_FREQ (32000000)

char encoder_click (char);
//volatile signed char encoder_count;
char encoder_count = 0;
unsigned char temp;
static unsigned char previous = 0;
char dir = 0;
//const signed char table[] = {0,-1,1,0,1,0,0,-1,-1,0,0,1,0,1,-1,0};
const char table[] = {0,0xff,1,0,1,0,0,0xff,0xff,0,0,1,0,1,0xff,0};

void main(void){
    PORTA=0;
    TRISA=0b00111000;
    ANSELA=0;   
    // SCS FOSC; SPLLEN disabled; IRCF 8MHz_HF;
    OSCCON = 0x70;      // set osc to 32MHz
    BORCON = 0x00;
    while(PLLR == 0);
    // WDTPS 1:65536; SWDTEN OFF;
    WDTCON = 0x16;
    WPUA = 0x00;
    OPTION_REGbits.nWPUEN = 1;
      
    while(1){
            
    dir = encoder_click(encoder_count);
      
        // attach led to RA0
    if (dir == 1){
        DDSdata = 1; // LATA0
        __delay_ms(100);
        DDSdata = 0; // LATA0
        return;
        }

    //attach led to RA2
    if (dir == 0xff){
        DDSclock = 1; // LATA2
        __delay_ms(100);
        DDSclock = 0; // LATA2
        return;
        }
    }
}

char encoder_click(char encoder_count)
{
    do {
    __delay_ms(2);
    
    temp = PORTA;     /* Read port - encoder on bits 3 & 4*/
    temp >>= 3;       /* Shift input to bit positions 1, 0 */
    temp &= 0x03;     /* Mask out bits */
    previous <<= 2;   /* shift the previous data left two places */
    previous |= temp; /* OR in the two new bits */
  
    encoder_count = table[(previous & 0x0f)];  /* Index into table */
        }while (encoder_count == 0);

    return(encoder_count);
}

 

[tumbleweed]

A few tips-

- move all the '#include' statements to after the '#pragma config' statements to avoid any possible conflicts.

- only use 'char' for characters. use 'int8_t' or 'uint8_t' for 8-bit values.

- the use of 'encoder_count' in the following statements likely isn't doing what you think.
You need to read about variable scoping and visibility.
Also, I moved the declarations for 'temp' and 'previous' so they're only visible to the routine that uses them.

char encoder_count = 0;                    // global (outside any function)

<snip>
dir = encoder_click(encoder_count);        // always passes the function a 0

<snip>
char encoder_click(char encoder_count)    // this 'encoder_count' isn't the same as the global one
{
    uint8_t temp;
    static uint8_t previous = 0;

    <snip>
    return(encoder_count);            // this doesn't update your global 'encoder_count'
}

 

[augustinetez]

- move all the '#include' statements to after the '#pragma config' statements to avoid any possible conflicts.

Thankyou.

- only use 'char' for characters. use 'int8_t' or 'uint8_t' for 8-bit values.

I did ask that question earlier and is how most of the texts I've been reading tell you to use them.

dir = encoder_click(encoder_count); // always passes the function a 0

Then what is being passed where to make the LED's light according to the direction the encoder is being turned?

Also, I moved the declarations for 'temp' and 'previous' so they're only visible to the routine that uses them.

I did try that and ended up with a load of warnings and build failure.

To be quite honest, I'm just about at the point of this being a total waste of time.

 

[tumbleweed]

Then what is being passed where to make the LED's light according to the direction the encoder is being turned?

You're not interested in passing anything to encoder_click(). All you're interested in is the return value of the function.
If you want to set encoder_count to the return value then use:

uint8_t encoder_click(void);

encoder_count = encoder_click();

 

[augustinetez]

Hmm, OK, I thought that line was reading from encoder_click rather than sending something to it.

Getting late here, so will have another look at it later tomorrow.

 

[tumbleweed]

It IS reading from encoder_click, but you have this:

dir = encoder_click(encoder_count);

which passes a copy of the global variable 'encoder_count' value to the function.
Your assignment statements in that function do NOT change the global variable 'encoder_count'.
You want to use a local variable inside the function to read the lookup table and return that value.

Time to read a book on basic C