timer peripheral and pin register

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

yet another timer pwm question

actually a couple of questions in fact.

I use a Amtega168. Timer1 is set to CTC mode with tops being ICR1. This as I wan t to use both OCR pins for fan control.

I run the processor at 20MHz. The PWM frequency is set to 25KHz. This is needed for PC fan speed control. I have checked and it runs at this speed.

I initialize the timer as follows:

/*****************************************************************************
 *
 *	Function name	: PWM_Timer1_Init
 *
 *	Returns			: None
 *
 *	Parameters		: None
 *
 *	Purpose			: function that initializes the selected timer to be a timer tick
 *					  timer with the settings as in the header file 
 ****************************************************************************/
void PWM_Timer1_Init(void)
{
	uint8_t l_Sreg;

	l_Sreg = SREG;		// store SREG state specially for global interrupt enable

	cli();				// clear interrupt 

	TCCR1A  = ((COM1A1_VAL<<COM1A1)| (COM1A0_VAL<<COM1A0)     		// OC0A toggle on compare match
	 		  |(COM1B1_VAL<<COM1B1)| (COM1B0_VAL<<COM1B0)		 	// OC0B toggle on compare match
			  | (0<<WGM11)|  (0<<WGM10));    						// Set mode 12 CTC top is ICR1

    TCCR1B  = ((0<<ICNC1)											// input capture noise cancel enable
			  |(0<<ICES1)            								// input capture edge select 0 = falling/ 1=rising
			  |(1<<WGM13)|(1<<WGM12)						        // Set mode 12 CTC top is ICR1
			  |(T1_CS2_VAL<<CS02)|(T1_CS1_VAL<<CS01)|(T1_CS0_VAL<<CS00)); 	// set division ratio according to mask 

    TCNT1   = 0;      												// clear timer/counter register

		// after configuring the timer to control the OC pins they default to low state writing TCCR1A register with COM bits
		// compare match should toggle the line after 
		// then input capture match should again toggle the line at he end of a period
	if(OC1A_OFF_LEVEL)
	{
		TCCR1C = (1<<FOC1A);	//when timer OC control is enabled the line starts low, force compare match to toggle the line to high level
	}
	if (OC1B_OFF_LEVEL)
	{
		TCCR1C = (1<<FOC1B);	//when timer OC control is enabled the line starts low, force compare match to toggle the line to high level
	}

    #if (PWM_TOP_VALUE+5) > 0xFFFF
     	// if the calculated value for the OCR1A register is to large error
		// by default PWM should be OFF. so OCR registers are set to top value +3
		// this ensures that line never change
       	#warning --------------------------
       	#error   PWM_TOP_VALUE value is to big for register. Change division ratio
       	#warning --------------------------
    #endif // PWM_TOP > 0xFFFF

	ICR1 = PWM_TOP_VALUE;			// set ICR1 register this is the top value of the counter and gives the frequency

	OCR1A = PWM_TOP_VALUE+2;	// set to value that never will be reached			
	OCR1B = PWM_TOP_VALUE+2;
	
	OCR1A_Value = OCR1A;			// store loaded values back in control variables
	OCR1B_Value = OCR1B;
	
	TIFR1 = 0xFF;	 			// Clear the TIFR0 register by writing all bits to one

	TIMSK1 = (1<<ICIE1)			// input capture interrupt enable 
			|(0<<OCIE1B)		// Output compare match B interrupt
			|(0<<OCIE1A)		// Output compare match A interrupt
			|(0<<TOIE0);		// Timer/Counter 1 Overflow interrupt enable

			// in CTC mode when counter reaches top(is ICR1 value) no overflow interrupt is given.
			// but the input compare match interrupt is given instead

	SREG = l_Sreg;				// restore SREG
}
	// PWM_Timer1_Init

I have set the COMxn pins to toggle on a compare match.

I have enabled the compare match interrupt. This interrupt is fired when the timer overflows.

in the ISR I do the following:

ISR (TIMER1_CAPT_vect)
{
	TCCR1C = (1<<FOC1A)|(1<<FOC1B);				// force Line back to start state for next pulse

	OCR1A = OCR1A_Value;							// set lines to next duty cycle value
	OCR1B = OCR1B_Value;	
}
	// ISR (TIMER1_CAPT_vect)

It seems though that when I get near 10% an 90% set duty cycle the interrupt sometimes mis fires. All of the sudden the PWM pulse inverts and I can only explain that by the fact that the write to the TCCR1C register comes to late.

Now I wonder if there is another way to make this variable duty cycle?
or is it better to first disable the clock inside the interrupt then toggle the line, update the OCR registers, clear the timer and then re-enable the clock?
doing the math indicates that the timer fires every 800 clock cycles, so I want to keep the ISR as short as possible.

I also wonder if it is possible to use the PIN register to check if the pin actually has the expected level. It should be such that after the TCCR register is written the line should always be high, so inside the ISR I could check the level and then decide to change the level.

I have a faint idea that I miss something. using variable duty cycle seems to be not usable without FW involvement inside the ISR and that is what I find a bit strange.

any information is appreciated.

regards

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

Why have you chosen mode #12?

Does your fan have to be 25kHz?
Or would 10kHz - 50kHz be good enough?

In which case, you just use regular PWM modes that update OCR1x values in hardware. And you don't need to worry about interrupts or glitches.

If 25kHz must be exact, would mode#8 or mode#14 be easier for you?

David.

p.s. I know we all have different styles of writing C. I would use less lines and briefer comments. Oh, some of your macros refer to Timer#0.

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

Hi David,

There is a specification document that describes that the fan speed control is 21-28 KHz. typical being 25KHz. A such I have choosen for the 25KHz.

As I control 2 fans independantly I choose to set the timer to ICR mode and then have the 2 OCR channels available to do the actual PWM.

missed the macro names will update that when I get home.

I miss the option to have the OCR pin being set/cleared when a compare match occurs and automatically cleared/set when the timer overflows. That would eliminate the need for having the interrupt at all. Then when I need updating of the duty cycle I just stop the timer update the OCR value and restart the timer(with then setting the lines to the correct state ofcoarse)

Note that this is for controlling 2 PC fans. There is a Intel specification on how they should be controlled. I could not find it quickly on the internet, and have it at home.

hope this helps a bit in understanding.

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

I am confused. With PWM#8 or PWM#14, you get hardware control of the OC1x pins.
You get the PWM frequency at exactly 25kHz and can alter the duty cycle whenever you want.

And you need no interrupts at all!

David.

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

OK, I think I then something totally mis understood in the datasheet....

I was under the assumption that the OCR lines only then were updated at 'bottom' and not when a compare match would occur. Seeing your reaction I guess that is not true?
the OCR pins are both changed on compare match(when the COM lines are set correctly) and also when the timer hits 'bottom'?

ok, before hitting submit I re-read the part on the fast PWm mode. I now really feel stupid that I missed this:

Quote:
The fast PWM differs from the other PWM options by its single-slope operation.
The counter counts from BOTTOM to TOP then restarts from BOTTOM. In non-inverting Compare Output mode,
the Output Compare (OC1x) is cleared on the compare match between TCNT1 and OCR1x, and set at BOTTOM.

So need to have another go this evening with this.....
Thanks for the info, I really totally missed out on that....

only thing that remains a question is how to safely update the OCR registers. I can do that atomicaly but will that ensure that when I update the registers it can not happen that the compare match is missed and thus the PWM duty cycle is inverted(or is it in this case the compare match did not happen, the line stays low and on the top to bottom transition just 1 PWM pulse is missed as the next time around the timer will fire normally again?

regards

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

Quote:

I was under the assumption that the OCR lines only then were updated at 'bottom' and not when a compare match would occur. Seeing your reaction I guess that is not true?
the OCR pins are both changed on compare match(when the COM lines are set correctly) and also when the timer hits 'bottom'?


There are COMxy bits that allow you to select the behaviour of the OC pins when compare matches occur so there's four possible variants (one of which is pin not connected to PWM so 3 really I guess).

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

Quote:

only thing that remains a question is how to safely update the OCR registers. I can do that atomicaly but will that ensure that when I update the registers it can not happen that the compare match is missed and thus the PWM duty cycle is inverted(or is it in this case the compare match did not happen, the line stays low and on the top to bottom transition just 1 PWM pulse is missed as the next time around the timer will fire normally again?

Once again, you will find the answer in the data sheet..

Why would the d/c be inverted? What might happen is that you get one "glitch" or a too long pulse. The timer will wrap to zero eventually and there the PWM signal is set high and then it will get to the compare match point where it will clear. Getting the PWM inverted would need both the wrap and compare match points to toggle the PWM signal. This is not the case - on compare match the signal is cleared and on the warp is is set.

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

JOhan,
a lot is happening here at the moment that is stressing me out, so my head is overrunning a bit and confidence is a bit hard to find.

i was looking for confirmation of this:

Quote:
on compare match the signal is cleared and on the warp is is set

thus that it can only be that a single cycle will be off.
in my current implementation they whole thing can get messed-up and thus inverted.

going to have some fun this evening and hopefully get this to work( it should not be that hard but in the current state you never know)

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

Quote:

i was looking for confirmation of this:
Quote:
on compare match the signal is cleared and on the warp is is set

It is in the text that you yourself quoted out of the data sheet!

Quote:
the Output Compare (OC1x) is cleared on the compare match between TCNT1 and OCR1x, and set at BOTTOM

Let's assume that you have OCR1A set to 100. The counter is currently at 80. Now you write the value 75 to OCR1A. The counter will continue counting. It will not hot the OCR1A value this time around. It will wrap to zero (i.e. hit the BOTTOM value). The signal is already set (high) so the effect of hitting BOTTOM this time will be none - the signal will simply remain high. Eventually the counter will count up to 75 whereupon the signal will be cleared (set to zero).

There will be no inversion. There will be one pulse that is too long (by a full cycle). This happens once for an update of OCR1x, but only under certain circumstances involving the current counter value and the new OCR1x value that you write.

Quote:
I was under the assumption that the OCR lines only then were updated at 'bottom' and not when a compare match would occur. Seeing your reaction I guess that is not true?

Again, you yourself quoted the data sheet:
Quote:
the Output Compare (OC1x) is cleared on the compare match between TCNT1 and OCR1x, and set at BOTTOM

Note "cleared" and "set". Two updates. Each a specific one. Not toggles.

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

Oops. PWM#8, PWM#10, PWM#14 all allow hardware PWM with an independently set period. And you can update OCR1x at TOP or BOTTOM depending on mode #.

The COM1xn bits control the set/clear/toggle behaviour.

All the same, I can't see much problem with the 'wait' to the safe time to change OCR1x. At least you never get a glitch while the timer wraps through 0.

David.

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

Quote:
I was under the assumption that the OCR lines only then were updated at 'bottom'
No, when you write a value to the OCRxx >>register<< the value does not take effect until bottom. The behavior of the OCxx >>output<< is controlled by the COMxx bits as Cliff stated.

Regards,
Steve A.

The Board helps those that help themselves.

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

Changing the PWM mode certainly made my life a lot easier. thanks all for the comments. At the moment I am glad that my head is firmly stuck on my body or else.....
on to the next challenge :) I do not seem to be able to measure more then 1 ADC channel. but that is a new problem and as I have had code that was able to measure 2 channels in the past I must have messed things up when tidying my code. So I have do a start from scratch there.
The most important thing was getting the PWM right and that is solved.
thanks again for all the help.

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

Quote:

2 channels in the past I must have messed things up when tidying my code.

The usual gotcha is a read_adc() that is passed a channel number that it simply OR's into the bottom of ADMUX. OR is great for setting more and more bits but you cannot clear them - you need an AND for that. Perhaps your read_adc() started with something like:

ADMUX &= 0xF8;

and you thought "what's the point of that?" and removed it.

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

Well I yet have to start investigating what is going on. To me it seems that there is as you say something wrong with the actual channel selection.

well my code is as follows:
initialisation:

    ADMUX =  REFSEL		    // reference selected
			|(0<<ADLAR);	// 0= right adjust(10bit), 1 is left adjust (8bit)

		// set the channel to the first one to be measured by default 
    g_ADC_Curr_Chan=0;
    ADMUX |= (pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]));
                            // select first channel

	ADCSRB = (0<<ACME)		// alternative pin for analog comparator. if 1 then ADC must be OFF
			 |ADTS_MASK;	// set trigger source as wanted

	ADCSRA	= (1<<ADEN)		// enable ADC converter
			 |(0<<ADSC)		// start conversion
			 |(0<<ADATE)	// enable trigger source
			 |(0<<ADIF)
			 |(0<<ADIE)		// enable ADC complete interrupt
			 |ADPS_MASK;	// set prescaler

    g_ADC_Interval = 0;     // clear the interval this makes sure that when the timer is 
							// accidentally started it only runs once.
	tempdif =0;

masks are set as:

After that all should be ready to do a ADC run

/*****************************************************************************
 * ADC SETTINGS
 ****************************************************************************/
#define REFSEL (1<<REFS1)|(1<<REFS0)
    /*
    	REFS1 REFS0 :
    	  0     0	: Aref pin
    	  0		1	: AVCC 
    	  1		0	: reserved
    	->1		1	: internal 1,1Vreference
    */

#define ADTS_MASK (0<<ADTS2)|(0<<ADTS1)|(0<<ADTS0)
    /*
    	TS2 TS1 TS0 : Trigger Source
       ->0   0   0  : Free running
    	 0   0	 1	: Analog comparator
    	 0   1   0	: INT0
    	 0	 1   1 	: T0 comp A match
    	 1   0   0	: T0 overflow
    	 1	 0	 1	: T1 comp B match
    	 1   1   0	: T1 overflow
    	 1	 1	 1	: T1 capture event
    */

#define ADPS_MASK (1<<ADPS2)|(0<<ADPS1)|(1<<ADPS0)
    /*
    	PS2 PS1 PS0 clock division factor
    	 0   0   0      2
	     0   0   1      2
    	 0   1   0      4
    	 0   1   1      8
    	 1   0   0     16
    	 1   0   1     32
    	 1   1   0     64
       ->1   1   1    128
    */

#define ADMUX_MASK (~( (1<<MUX3)|(1<<MUX2)|(1<<MUX1)|(1<<MUX0) ) )
    // mask to mask out the mux bits to be able to set a new channel

    // mux bits settings for different channels
#define ADC_PC0 0x00
#define ADC_PC1 0x01
#define ADC_PC2 0x02
#define ADC_PC3 0x03
#define ADC_PC4 0x04
#define ADC_PC5 0x05
#define ADC_PC6 0x06 // only available in QFP 
#define ADC_PC7 0x07 // only available in QFP 
#define ADC_1V1 0x0E
#define ADC_GND 0x0F

with this change that in my latest version code the ADPS is set to 111(div128 iso div32)

further defines are:

#define NR_OF_CHANS 2
#define AREF_CPU 1100UL // 1,1V reference in millivolts
volatile uint16_t ADC_Results[NR_OF_CHANS];    // place holder for ADC results 
const uint8_t ADMUX_CHAN_LIST[NR_OF_CHANS] PROGMEM  = {ADC_PC6,ADC_PC7}; 

now when I want to run a conversion sequence ( once per second) I call:

void ADC_StartSequence(void)
{
#if NR_OF_CHANS > 1
	// if the number of channels defined and thus used is larger then 1 before measuring set to the first channel
        // Set indicator to first channel in list
    g_ADC_Curr_Chan =0;
	ADMUX &= ADMUX_MASK; // clear admux bits
    ADMUX |= (pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan])); 
    // set to first channel in list
#endif
    
   	ADCSRA |= ((1<<ADSC)|(1<<ADIF));	// start first conversion and clear interrupt flag 
    ADCSRA |= (1<<ADIE);				// enable interrupt to handle the other channels
}
	// ADC_StartSequence

this should trigger the first conversion. When that is done the ADC interrupt fires.
this is what I do in there:

ISR(ADC_vect)
{
    tEvent l_CurrEvent;


	
#if NR_OF_CHANS >1
	if (tempdif ==1)
	{
		ADC_Results[g_ADC_Curr_Chan] = ADC; // store measurement result
	
		g_ADC_Curr_Chan++;	// increment the channel list pointer
			// check if not at the end of the measurements
	    if (g_ADC_Curr_Chan == NR_OF_CHANS)
		{ // end of measurements set back to the beginning
			g_ADC_Curr_Chan = 0;
	    }
    
            // Set channel ready for the next measurement
		ADMUX &= ADMUX_MASK; // clear admux bits
	    ADMUX |= pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]);
		tempdif =0;
	}
	else
	{
		tempdif++;
	}
	
        // check if at the end of measurements
    if(g_ADC_Curr_Chan == 0)
    { // all channels done so stop
        ADCSRA &= ~(1<<ADIE);   // disable interrupt  
        l_CurrEvent.Event = ADC_DONE_EVENT;
        l_CurrEvent.Data = 0;
        Event_Add(&l_CurrEvent);
    }
    else
    {   // start getting next channel
        ADCSRA |= (1<<ADSC);	// start conversion    
    }

#else
			// if only one channel has to be measured just put event on que after each conversion
        l_CurrEvent.Event = ADC_DONE_EVENT;
        l_CurrEvent.Data = 0;
        Event_Add(&l_CurrEvent);
#endif

}
	// ISR(ADC_vect)

I added tempdif because it seemed that the first reading on a new channel was garbage. So I take 2 readings and only save the second one.
After the second reading the channel should be changed and a new conversion started, that will also be done twice and the second conversion saved. After that the ADC is done and I put an event on the que that it is done.
then in the event handler I process the results.

void ConvertRawTempToDegrees(void)
{
	uint32_t l_Calc;
	
	// first copy the data set to the global structures
	cli();	// need to read the ADC result atomic to ensure it is not overwritten halfway through the read
	g_FrontSensor_Data.Raw = ADC_Results[ADC_FRONT_SENSOR];
	g_BackSensor_Data.Raw  = ADC_Results[ADC_REAR_SENSOR];
	sei();	// re-enable the interrupts

	l_Calc = (uint32_t)g_FrontSensor_Data.Raw;		// copy raw value to working register
	l_Calc = (l_Calc * AREF_CPU) / 1024; // convert ADC value to Voltage level in millivolt
	if ( (l_Calc % 10) > 4 )
	{
		l_Calc +=10;	// as last digit was higher than 4 increase with one
	}
	l_Calc /= 10; // remove the last digit so in 10mv resolution which is more then enough this give temp erature in degrees
	g_FrontSensor_Data.Dgc = (uint16_t)l_Calc; // copy to display register


	l_Calc = (uint32_t)g_BackSensor_Data.Raw;		// copy raw value to working register
	l_Calc = (l_Calc * AREF_CPU) / 1024; // convert ADC value to Voltage level in millivolt
	if ( (l_Calc % 10) > 4 )
	{
		l_Calc +=10;	// as last digit was higher than 4 increase with one
	}
	l_Calc /= 10; // remove the last digit so in 10mv resolution which is more then enough this give temp erature in degrees
	g_BackSensor_Data.Dgc = (uint16_t)l_Calc; // copy to display register
}
	// ConvertRawTempToDegrees

I get readings that seem to be in the neighborhood, but sometimes when I re-flash the code they are off by 10 degrees in both directions.
I read-out a LM35 temp sensor. when putting a voltage meter on that it gives me the right voltage. the ADC readings seem to be off.
but as said I have to start investigating what is going on and see if it is hardware(that at the moment I also do not fully rule out, although multiple versions boards have this problem(being feroboard handwork with wires and thru hole resistors and a PCB custom made with SMD components)

at this point I do not see anything being wrong, but this evening I hope to dig into the ADC again, starting from scratch not using interrupts and only doing ADC to see if things work out then.

the changed PWM settings have been a great help, they at least made the code much easier to understand :)

regards

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
   ADMUX &= ADMUX_MASK; // clear admux bits

Looks like my guess was wrong (though I haven't actually worked out what ADMUX_MASK is - oh how I do love obtuse macros hiding stuff!).

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

I just looked at several pages of source code to initialise 3 SFRs.

I did not even read them!

So it really comes down to your particular coding style. Yes, on very complex chips, a few pages of source code lives inside a library or Wizard. You read the HTML documentation and obey the rules. The Library/Wizard author has already debugged perfectly.

Somehow, 3 lines with a brief comment seems more appropriate for a simple chip like an AVR.

David.

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

clawson wrote:

   ADMUX &= ADMUX_MASK; // clear admux bits

Looks like my guess was wrong (though I haven't actually worked out what ADMUX_MASK is - oh how I do love obtuse macros hiding stuff!).

#define ADMUX_MASK (~( (1<<MUX3)|(1<<MUX2)|(1<<MUX1)|(1<<MUX0) ) )
    // mask to mask out the mux bits to be able to set a new channel 

Quote:
So it really comes down to your particular coding style. Yes, on very complex chips, a few pages of source code lives inside a library or Wizard. You read the HTML documentation and obey the rules. The Library/Wizard author has already debugged perfectly.

At the moment to me the AVR seems to be a very complex chip as things just do not seem to work ;)

A long long time ago I have set things up such that I could use one set of files for all AVRs I was playing with. In the end that never came of the ground. that is why I have put a number of things in macro defines. I now am slowly pulling things apart again making separate files for each AVR again.

with respect to this ADC problem I know I have had multiple channels working so it must be some simple thing that does not work as I expect.

with respect to adding comments. As I am not dealing with this on a daily basis I have added a lot of comments just to make sure for myself that after a couple of months I would still understand why I have done what. I think to people who are doing this on a daily basis that looks over done and can be done simpler, but as it might be that I am working on it tonight and then 2 weeks have other tings I tend to spend then a lot of time spitting through the code finding out again what I have done .

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

Yes I already saw that overly complex looking macro - I didn't care to try and decode what it's doing so I still don't know. Personally when I want a mask I write something like:

#define MASK (0xF8)

and know this will clear the bottom 3 bits or

#define MASK (0xB7)

and know this will clear bits 6 and 3 or whatever.

I suppose if the MUXn bits were not adjacent there'd be an argument for building the mask from the individual bits.

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

ok, been busy for a while finding the problem, but it turned out to be a hardware problem. I had added a RC filter on a line and that caused the sensor (LM35) to oscillate removing the Capa was enough to get good stable readings. Now I can start re-building my code to using interrupts.
Glad this did not turn out a FW issue or I probably would have gone mad.... atleast, that is what I hope now as I started from scratch with just a very basic basic few lines of code that will set channel do conversion and wait for result and then dump that directly to the LCD

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

after writing this could not resist and build back the original stuff, although a lot less, there still seems to be a offset in read-out when using the interrupt stuff compared to just set channel and wait for it to be done, then set next channel and wait again.
so this is not entirely done...

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

Surely your ISR(ADC_vect) will read the current ADCW and store it in a global location. Then set the ADMUX for the next channel, and start another conversion.

Yes, you may want to arrange your code so that there are a few instructions between changing ADMUX and the ADSC is started.

Equally well, you can use a timer interrupt to change ADC channels.

The last thing that you ever want to do is 'busy-wait' for the whole ADC conversion to complete.

With polling, you busy-wait before reading the 'previous' result, then starting the peripheral for the 'next'.

David.

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

David, indirect I now use a timer.
When a 1second timer expires, I start the first conversion and enable interrupts. then when the first conversion is finished I enter the interrupt, there I set the next channel, start the conversion and exit again. when all channels are done I put an event on the que and there I process all measurement results.
when just plain waiting for the conversions to start all read-outs are OK and as expected. but when doing it through interrupts measurements seem to produce different results. In the end yesterday I found out that I had a HW problem, when that was solved the 'wait until finished' code worked. after that I just quickly changed the code back to the old interrupt driven method and the code failed again. But then I did not have time to dig into what happens. Hopefully I have some time this evening and I can sort that problem too. I can not imagine that it should not work, so it probably is something stupid I have done or something that I expect to do something, but instead does something completely different...

regards

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

OK, have been doing some more digging.
The problem is always in the first reading. doing 10 reading, the first one is always off. even with channel changing. If I swap measurement channel 1 and 2 the fault switched with it (so again the first measured channel is off)

I changed my interrupt to take 2 measurements on each channel and always discard the first measurement. much like I posted before.

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

Quote:

The problem is always in the first reading. doing 10 reading, the first one is always off. even with channel changing.

This, perhaps?:
Quote:
In Free Running mode, always select the channel before starting the first conversion. The channel selection may be changed one ADC clock cycle after writing one to ADSC. However, the simplest method is to wait for the first conversion to complete, and then change the channel selection. Since the next conversion has already started automatically, the next result will reflect the previous channel selection. Subsequent conversions will reflect the new channel selection.

mega168 data sheet, page 250.

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

JOhan, is that not only valid when you have enabled external trigger sources through the ADATE bit in the ADCSRA register?

I might be wrong here, but I am under the assumption that I use single conversion mode with the ADC complete interrupt enabled.
what I do (just found that I forgot to copy the code to my memorystick so no code to show) is:
- when a timer expires I call a function to start the ADC. Inside that I first set the ADMUX to the first channel(ADC6) Then start a conversion and in the same instruction I clear the ADIF bit by writing to it(I write a one to it to clear it just in case a false interrupt is pending) then in the next instruction I enable the interrupt.
end function
then inside the ISR
first thing I did was copy the ADC register to a working/storage register, then , when not all channels are done I set the next channel and start a new conversion. When all channels are done I reset the channel to the first channel, and disable the interrupt.

when doing 4 conversions like this(ad6, ad6, ad7,ad7) the first ad6 reading was a large number of times off compared to the second(not 1 or 2 but 40 or 50) . I also did 4x ad6 and 4xad7 without the ADMUX channel change and always only the first reading was off.

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

You are using the 1.1V internal reference:

In the datasheet, Atmel wrote:
When the bandgap reference voltage is used as input to the ADC, it will take a certain time for the voltage to stabilize. If not stabilized, the first value read after the first conversion may be wrong.
In table 29-16, Atmel wrote:
Bandgap reference start-up time
VCC=2.7
TA=25°C
typical:40 μs
maximum:70 μs
You've selected an ADC prescaler of 32. At 20 MHz, the first conversion will take 25*32*50 = 40,000 ns = 40 μs.

While the internal reference might be stable by then (it could take 70 μs), it most certainly isn't when the conversion begins with the MSB after 13.5 ADC cycles, or 21.6 μs.

Set ADEN
Set REFS for 1.1V and first channel
Wait for at least 70 μs
Initiate first conversion

Note also that with a prescaler of 32 @ 20 MHz, the ADC clock will be 625 kHz. That will reduce the usable resolution by 1 or 2 bits.

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

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

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

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

"Fast.  Cheap.  Good.  Pick two."

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

 

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

joey,
thanks for the explanation, but....
I initialize the ADC during start-up. after that I display a boot screen for 2 seconds and only after that I start measuring. the ADC never gets turned off (ADEN always set)
what I do is take a set of measurements every second. After my initial code post I changed the divider to 1/128 giving just below 200KHz ADC clock. The datasheet stated that then the resolution is good. but even if I loose 1 bit that would not be a ADC count of 50. I have done some testing and it gave me a range of 300 to 400 (decimal) were I expected values of about 350 for the first measurement. all consecutive measurements gave a range of 345 - 355 were of coarse there will be a very slight variation as the temperature will drift a bit and the reference will drift a bit.
or is loosing a bit that big an impact?

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

meslomp wrote:
I initialize the ADC during start-up.
Including ADMUX? Do you set REFS[1:0] after ADEN?

Quote:
I have done some testing and it gave me a range of 300 to 400 (decimal) were I expected values of about 350 for the first measurement.
Do you mean the first measurement in a given group of measurements? Or only the very first measurement after start-up?

In the datasheet, Atmel wrote:
The first ADC conversion result after switching reference voltage source may be inaccurate, and the user is advised to discard this result.

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

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

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

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

"Fast.  Cheap.  Good.  Pick two."

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

 

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

JOeymorin this is my init routine:

void ADC_Init(void)
{
    ADMUX =  REFSEL_MASK    // reference selected
			|(0<<ADLAR);	// 0= right adjust(10bit), 1 is left adjust (8bit)

		// set the channel to the first one to be measured by default 
    g_ADC_Curr_Chan=0;
	
    ADMUX |= (pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]));

	ADCSRB = (0<<ACME)		// alternative pin for analog comparator. if 1 then ADC must be OFF
			 |ADTS_MASK;	// set trigger source as wanted

	ADCSRA	= (1<<ADEN)		// enable ADC converter
			 |(0<<ADSC)		// start conversion
			 |(0<<ADATE)	// enable auto trigger 
			 |(0<<ADIF)		// interrupt flag
			 |(0<<ADIE)		// enable ADC complete interrupt
			 |ADPS_MASK;	// set prescaler

    g_ADC_Interval = 0;     // clear the interval this makes sure that when the timer is 
							// accidentally started it only runs once.
}

this is called somewhere during all my initialisations. After init I wait for 2 seconds to show a bootscreen and also to ensure a start of the fans(according to intel spec this should ensure that whatever fan is connected it will start).
After the 2 seconds I start a 1 second timer for ADC measurements. whenever that timer expires I do the following:

void ADC_StartSequence(void)
{
        // Set indicator to first channel in list
    g_ADC_Curr_Chan =0;
	ADMUX &= (~(ADMUX_MASK)); 
    ADMUX |= (pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]));	// set first channel to measure
 	
	ADCSRA |= ((1<<ADSC)|(1<<ADIF));	// start conversion and clear interrupt flag
    ADCSRA |= (1<<ADIE);				// enable interrupt to handle the other channels
}

This starts the first conversion the others are done in the interrupt routine:

ISR(ADC_vect)
{
	tEvent l_CurrEvent;

	static uint8_t l_SampleTaken = 0;
	
	if (l_SampleTaken >=1)
	{
		ADC_Results[g_ADC_Curr_Chan] = ADC;			// store measurement result
		
		g_ADC_Curr_Chan++;							// increment the channel list pointer

		if (g_ADC_Curr_Chan >= NR_OF_CHANS)
		{ 
				// all channels done
			g_ADC_Curr_Chan = 0;					// re-set channel number

			ADCSRA &= ~(1<<ADIE);					// disable ADC complete interrupt
			
			l_CurrEvent.Event = ADC_DONE_EVENT;
			Event_Add(&l_CurrEvent);				// put event on que
		}
		
		ADMUX &= (~(ADMUX_MASK)); 
		ADMUX |= pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]);	// set next channel
		
		l_SampleTaken =0;

			// check if at the end of measurements
		if(g_ADC_Curr_Chan != 0)
		{   
			ADCSRA |= (1<<ADSC);	// not done start next conversion
		}
	}
	else
	{
		l_SampleTaken++;
		ADCSRA |= (1<<ADSC);	// not done start next conversion		
	}
}

I mean the first measurement in a given group of measurements. and thus not only the first measurement after start.
hope you spot something.

regards

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

I don't think this is your issue, but have you declared g_ADC_Curr_Chan to be volatile?

Your most recent code discards the first conversion for each channel. How are you determining that this first conversion is inaccurate if you're not reading it? I assume you made this determination using code you haven't posted.

By the way, you could simplify your code a bit by using __flash instead of PROGMEM and pgm_read_byte().

What is the impedance of your temperature sensor?

In a recent post I mentioned some findings I'd made w.r.t. the S/H circuitry. It may be relevant here.

The way you've coded things, you change the MUX immediately before the conversion which will use it. Better to change is as soon as the previous conversion is initiated and wait a period of time. This will leave the S/H circuitry open to the channel for longer before a the hold at the start of the conversion. If the channel has a higher impedance this will help ensure that the S/H cap reaches equilibrium with the channel input.

I'd suggest you dispense with the ADC interrupt and use only a timer interrupt (or other timing mechanism):

// pseudocode
TIMER ISR {
  read ADC from last conversion and store
  initiate new conversion
  change MUX for next channel
}

Run this, say, every half-second. For two channels, this will give you a 1-second interval between samples for each channel, but they will be 0.5 seconds out of phase.

This will muck with your event scheme, but you can sort that out pretty easily. Separate events for each channel, or live with the phase difference.

You also want to make sure you're clear on which channel's conversion you're reading. It will be the one before the one which is currently set.

I would also do this:

ADMUX &= (~(ADMUX_MASK));
ADMUX |= pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]);

... in one step:

ADMUX = (ADMUX & ~(ADMUX_MASK)) | pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]);

This avoids needlessly setting the channel to ADC0 before changing it again to your desired channel, and is a teeny bit faster since there's only one read and one write of the volatile ADMUX.

////////////////////////////////////////////////////////////////////////////////////

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

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

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

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

"Fast.  Cheap.  Good.  Pick two."

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

 

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

clawson wrote:
Yes I already saw that overly complex looking macro - I didn't care to try and decode what it's doing so I still don't know. Personally when I want a mask I write something like:

#define MASK (0xF8)

and know this will clear the bottom 3 bits or

#define MASK (0xB7)

and know this will clear bits 6 and 3 or whatever.

Ouch. I wouldn't use either one.
For one thing, I'd not use a use a negative mask.
For another, they entirely obfuscate their relationship to what one is trying to do.
For masking MUX bits, I'd #define something to (7<<MUX0).
For the other I'd use something like mk_mask2(fred, greg).
fred and greg would be 3 and 6 if I really had nothing better,
but I'd try to avoid using magic numbers.
I'll leave #define-ing mk_mask2 as an exercise for the reader.
Quote:
I suppose if the MUXn bits were not adjacent there'd be an argument for building the mask from the individual bits.
Even then, I'm not so sure that it should be done explicitly in the application's source code.
I'd be inclined to use something like mk_mask3(MUX0, MUX1, MUX2),
where mk_mask3 is a tried, true, trusted and trustworthy macro #defined in a "library" header.
Of course, absent the four t's, the source would become less readable, not more.

"SCSI is NOT magic. There are *fundamental technical
reasons* why it is necessary to sacrifice a young
goat to your SCSI chain now and then." -- John Woods

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

Joey, I will change my code and see what happens.
The finding of the s+h gate being open all the time is really interesting.
I have put a 10R resistor in series with my LM35 output pin. IIRC the input of the ADC is 1K1 into 14pF. I did not expect the 10R to be much of an issue there.
The code posted was indeed the latest version, but the original code was all the active stuff without the addition of the "sampletaken" check. so assume sample taken to be always one there.

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

OK, think Joey gets the points......
All I now did was make the samples just one:

if (l_SampleTaken >=0)

and replaced the double ADMUX Instructions with:

ADMUX = (ADMUX & ~(ADMUX_MASK)) | pgm_read_byte(&ADMUX_CHAN_LIST[g_ADC_Curr_Chan]);

and now things seem to be working normal again, so it looks like changing the ADMUX in 2 steps gives a problem. It almost seems to be rather by accident that the second channel and the second reading always was correct.

going to strip out the samples code and see what happens then.