New C-user need help with "indexed" port pin selection.

[Fijvv]

/* How to define / use RELE[Ch]  == PTED[Ch]??   instead of program below?       */

/* to get program shorted and more flexible */

/*      #define RELE[Ch] PTED_PTED[Ch]  ???*/

/* Where Ch can have value 0 to 3 and examble RELE[1] drive port E pin PTED1 and RELE[2] drive  pin PTED2 */

/****************************************************/
/* OptoTest     with        QE128                            */
/* Opto active Low      */
/* If OptoLow[Ch] value is bigger than OffTime[Ch]  then set Port E pin 1 high */

/* Ch = Channel (for input , output and memory locations */
/* RELE can have value ON ==1 or OFF==0*/

void OptoTest(void) {
  if (Ch==1){
    
    if (RELE1 == OFF){
       
       if (OptoLow[Ch] >= OffTime[Ch]){
          //OffTime =Measured pulse time
          RELE1 = ON ;
          OptoLow[Ch] = 0;
       }
       
       if (OPTO[Ch] == OFF) {
          OptoLow[Ch]++ ;
       }
    }
  }
  if (Ch==2){
    
    if (RELE2 == OFF){
       
       if (OptoLow[Ch] >= OffTime[Ch]){
          //OffTime =Measured pulse time
          RELE2 = ON ;
          OptoLow[Ch] = 0;
       }
       
       if (OPTO[Ch] == OFF) {
          OptoLow[Ch]++ ;
       }
    }
  }
     
  else OptoLow[Ch] = 0;
}
   
 
/****************************************************/

[Lundin]

A generic version would look something like this:

#define RELE_N 2/* Assuming RELEn are some sort of hardware registers. */typedef volatile uint8* RelePtr;const RelePtr RELE [RELE_N] ={  (RelePtr) 0x1234,  (RelePtr) 0x1235,};void OptoTest (uint8 n){  if(n < RELE_N)  {    if(*RELE[n] == OFF)    {      if(OptoLow[n] >= OffTime[n])      {        *RELE[n] = ON;        OptoLow[n] = 0;      }      else if(OPTO[n] == OFF)      {        OptoLow[n]++;      }    }  }  else  /* this else statement from the original code seems highly suspicious */  {    OptoLow[n] = 0;  }}

 - The else statement pointed out above seems highly suspicious, it is most likely a bug in the original code, and thus it will be a bug in this code as well.

- Note that this code uses 0-indexed arrays as done in the C language, instead of 1-indexed arrays.

- Avoid using global variables. They lead to tons of problems, especially in embedded systems. There are vey few cases where you need global variables, the only valid case is for declaring hardware registers.

[bigmac]

Hello Lundin,

My understanding is that the OP required to index the individual bits of a hardware register.  I cannot see how your generic version achieves this - perhaps I am missing something.

---

Lundin wrote:

- Avoid using global variables. They lead to tons of problems, especially in embedded systems. There are vey few cases where you need global variables, the only valid case is for declaring hardware registers.

---

This seems a rather provocative assertion.  How would you communicate a value derived within an ISR, to be used within the main loop, or vice versa, if not by means of a global variable?

Regards,

Mac

[Lundin]

> My understanding is that the OP required to index the individual bits of a hardware register. I cannot see how your generic version achieves this - perhaps I am missing something.

 As others had already responded to that, I did not I actually have no idea what the code I wrote is supposed to do, I merely showed how to make it generic. The question is what "ON" and "OFF" are. If they are bit masks, the code won't work. If they are byte values the code will work. This is not made clear by the OP.

>> - Avoid using global variables. They lead to tons of problems, especially in embedded systems. There are vey few cases where you need global variables, the only valid case is for declaring hardware registers.

> This seems a rather provocative assertion. How would you communicate a value derived within an ISR, to be used within the main loop, or vice versa, if not by means of a global variable?

 By a static file scope variable:

static volatile BOOL I_am_communicating_with_the_ISR;

This variable is not global, but only accessible from the same source file.

I am quite sure that my provocative statement is true. At least none has been able to prove it wrong so far .

[Fijvv]

Hi Fellows

Is it really so that C-language make's life very difficult??

Just a simple case using asm. code, will course so much disscussion....

... and I am little bit confused now...how I can or would do my task without any bigger bugs in it

Same program I made using assembler in 40 hours and it took all 1,2kb EEPROM space in a tiny 705 and working well.

Now I have spent over a year trying translate it to the C. About half of it  works OK, with only one channel.

I have used more than 4kb flash allready...

But just now biggest problem is this port bit selection (bit manibulation) and use of index (or what you call it in C)

RELE(X) / X,PTED

It might be same problem when I try to change ADC-channel

Does this work?

/****************************************************/
/* ADC_Measure                                      */

void ADC_Measure(void){
/*AIEN conversion complete interrupts = 0x40 in ADCSC1 */
  ADPortNumber = (0x40+Ch);
   
  ADCSC1 = ADPortNumber;/* Start measurement of next channel */
  while (!ADCSC1_COCO); /* wait untill ready*/
  //Temp_AD_Result = ADCR;
  ADCResult[Ch] =  ADCRL/10;  
  if (Ch > 0) {     //Do not reset if TimeSet channel
    if (ADCResult[Ch]>=24)
    MCU_RESET();

Or must I really write (copy and rename) all four time's or as many time as I have channels?

(and having big possibility to forget same renaming)

[Lundin]

> Is it really so that C-language make's life very difficult??

Yes. However, there are no sensible alternatives.

> ... and I am little bit confused now...

Here is a good newbie tutorial for embedded C: http://users.ece.utexas.edu/~valvano/embed/toc1.htm

It is for the old 68HC12, but the methods are pretty much the same for any microcontrollers.

[Fijvv]

Thank's Lundin,

That's also what I have looked for

[rocco]

---

Fijvv wrote:

But just now biggest problem is this port bit selection (bit manipulation) and use of index (or what you call it in C)

RELE(X) / X,PTED

It might be same problem when I try to change ADC-channel

---

There is a major architectural difference between the two:

In the S08 family "bit-set", "bit-clear", "branch-if-set" and "branch-if-clear" instructions all have the bit number embedded in the opcode. The instruction to test bit-0 is different from the instruction to test bit-1. That makes it difficult to use an index with it. The same was true with the old 705.

With the ADC, the channel is simply a number that you write into a register. There isn't a problem with writing an index into the channel register.

[bigmac]

Hello,

Yes, some operations are more difficult to code in C, compared with assembler, whislt other operations are more straight forward using C.  However, there should be little difficulty in doing what you require.

I think that some of the confusion is because you originally seemed to be implying that you required a suitable preprocessor macro, i.e. #define ...  It is the macros that are very limited in their application, and cannot utilize the value of a runtime variable.  Such limitations do not apply to the use of C functions.

Consider the following functions that use an index value to any of the port E pins:

// Set or clear a Port E pinvoid RELE( byte index, byte state){  byte mask;  mask = 1 << index;  if (state)    PTED |= mask;   // Set port E pin  else    PTED &= ~mask;  // Clear port E pin}// Test a Port E pinbyte RELE_test( byte index){  byte mask;  mask = 1 << index;  if (PTED & mask)    return 1;       // Pin is high  else    return 0;       // Pin is low}

These functions can then be utilised in a further function that processes a single Opto channel in accordance with your original coding.

#define ON  1  // These are macros#define OFF 0// Process Opto channelvoid Opto_proc( byte n){  if (RELE_test(n) == OFF) {  // Test output state    if (OptoLow[n] >= OffTime[n]) {      RELE( n, ON);  // Set output state to ON      OptoLow[n] = 0;    }    if (OPTO[n] == OFF)      OptoLow[ch]++;  }}

This function can then be called to process whatever channels you require.

e.g.if (Ch <= 2)  Opto_proc( Ch);else  OptoLow[Ch] = 0;

You can use a similar principle for your ADC code.  However, I think that your posted code is a little confused.  You appear to have enabled the ADC conversion complete interrupt, and then proceed to poll the COCO flag.  And you don't appear to have any ISR code.

I assume that you require to cycle through the various ADC channels, but the approach taken will depend on the sampling frequency required.  If you do not require the maximum sampling rate, but require to space the samples at regular intervals, the use of a timer interrupt, rather than the ADC interrupt, would be more appropriate (with the interrupt period exceeding the ADC conversion time).  The procedure within the ISR (timer or ADC) would be to:

1. Read the result of the previous conversion, and store the result global variable or array.
2. Increment a counter variable to the next channel, and start the next conversion on this channel.
3. Maybe set a flag to externally indicate outside of the ISR, that a new conversion is available.

Any global variables that are referenced both within the ISR and in the main loop will need to be defined as volatile.  Do any processing of the raw ADC result within the main loop, so that the ISR executes as quickly as possible.

Regards,

Mac

[Fijvv]

Hi,

This clarify a lot!

Now I understand, little bit better anyway

Thank's!

[irob]

---

bigmac wrote:

This seems a rather provocative assertion.  How would you communicate a value derived within an ISR, to be used within the main loop, or vice versa, if not by means of a global variable?

---

I agree!  And even beyond ISR, in a straight polling app with large state machines, setting and clearing flags globally is a must.

[Lundin]

It seems most people do not know what a global variable is. A global is a variable made accessable to the whole program. Example:

int x;

...

extern int x; /* this is fine */

This is different from a local file scope variable, which is not visible to the whole program:

static int x;

...

extern int x; /* this wont link */

So no, such variables shouldn't be global (global scope), they should be static (file scope). C has plenty of scopes, and the global scope should never be used, with the sole exception of hardware registers.

[bigmac]

Hello Lundin,

No doubt you will correct me if I am wrong. 

My understanding is that, in the file where int x; is defined as a global variable, it will have visibility throughout that file.  However, it would not have visibility in any other file unless the declaration extern int x; is made within those files.  If this be correct, it would seem that the actual visibility of a global variable is within the programmer's control.

The use of static variables would mean that all functions that use any of the data associated with an ISR, would require to reside in the same file as the ISR.  It is also possible that the data associated with more than one ISR may be required by a function, or where there are a multiplicity of variables handled within say, a timer ISR, this could result in an excessive file size. .

One alternative would be to create separate functions exclusively to read and write each static variable.  For a small imbedded project using an 8-bit MCU with very limited resources, this extra coding complication would be hard to justify, not to mention the additional cycles required by the extra function calls.

I think that a certain amount of pragmatism is required where the code size limit is say, 8K bytes.

Regards,

Mac

[Fijvv]

Hello,

In this I strongly agree with Mac!

In smaller programs global variables make's it much easier to handle and there isn't any big risk, or no risk att all, or even less than using local variables (with perhaps same name).

Usually these programs have only one programmer.

Actually I haven't found any reason to use local variables in these tiny programs, if  the mcu have enough ram.

(if not, I can use "TEMP" variable's as well. clr temp, use, clr temp....

Also in most cases sub routines have something to return for  the main program.

jvv

[rocco]

---

Fijvv wrote:

Actually I haven't found any reason to use local variables in these tiny programs, if  the mcu have enough ram.

---

The best reason for using local variables is not having much ram.

Local variables are created on the stack only as needed by the function that uses them. When your code is not executing that function, they don't exist at all, and therefore use no ram.

Local variables are one of the best ways to maximize your use of ram. They also allow for recursive functions, which can help to simplify algorithms, execute faster, or use less code.

[Fijvv]

Many thank's bicmac!

I have used asm. decades and everybody said, please start using C, don't need asm. anymore....

Bye the way does that work with ColdFire too?

People from Freescale, who introdused QE128 and MCF51, said that with ColdFire I can't use asm. ?

[bigmac]

Hello,

Where inline assembly code is used within a macro, or elswhere for that matter, it does mean that the code will be specific to a particular derivative family.  Alternative macros would need to be defined for Coldfire devices.  As I am not familiar with Coldfire, I cannot comment further.

Further to Rocco's comment, my understanding is that all macros need to be fully resolved by the preprocessor, prior to compile time, to provide a simple text substitution.  The use of macros would be considered a "cosmetic" change only to produce a more readable code for a better functional understanding.

If you need more flexibility, and portability, you would need to create a function, certainly requiring more complex code.

void RELE( byte index, byte state){  byte mask;  mask = 1 << index;  if (state)    PTED |= mask;   // Set port E bit  else    PTED &= ~mask;  // Clear port E bit}

Regards,

Mac

[bigmac]

Hello,

Handled as two separate macros, one to turn a bit on, and a separate one to turn a bit off.  Maybe the following macros may do what you wish.  Of course there would be no bounds checking for the bit number.

#define RELE_ON(x)   __asm bset x,PTED

#define RELE_OFF(x)  __asm bclr x,PTED

Regards,

Mac

[rocco]

---

bigmac wrote:

#define RELE_ON(x)   __asm bset x,PTED

#define RELE_OFF(x)  __asm bclr x,PTED

---

Of course, "x" in the above case is a compile-time variable, not a run-time variable.

You can NOT do this:

for ( x=1 ; <=4 ; x++ ) {

    RELE_ON(x)

}

"x" will be resolved at complie-time, and will be a constant at run-time.

I think the best way to select the bit at run-time is to use a case statement.

[Fijvv]

rocco,

Please give a examble