AVR Freaks

BTW your whole code looks like it could benefit from some modularity. When you have a few lines that look like they "belong together" consider putting that into a function and calling it. For example:

    //***************** ADC0 is used here and is set by default *******************
    ADMUX |= 1 << REFS0; // This sets our reference to the 5v input rail
	ADMUX |= 1 << ADLAR; // This moves the result into the high byte of ADCH for 8 bit
	ADCSRA |= 1 << ADEN; // This Enables the ADC

looks to me like:

void ADC_init() {
    //***************** ADC0 is used here and is set by default *******************
    ADMUX |= 1 << REFS0; // This sets our reference to the 5v input rail
	ADMUX |= 1 << ADLAR; // This moves the result into the high byte of ADCH for 8 bit
	ADCSRA |= 1 << ADEN; // This Enables the ADC    
}

But for the love and honour of God don't go over-board with the |= use. Just use = when you can. There are two lines setting bits in ADMUX there - that could be done in one line with =.

 

Similarly:

	//********* This is setting up USART to send the potentiometer value serialy ***
	UBRR0 = 12; // this sets baud rate at 9600
	UCSR0A = (1 << U2X0); // this doubles data rate in asynchronous mode
	UCSR0B = (1 << TXEN0); // This enables transmit
	UCSR0C = (1 << UCSZ01) | (1 << UCSZ00); // This sets character size to 8 bits

looks to me like:

void UART_init(void) {
	//********* This is setting up USART to send the potentiometer value serialy ***
	UBRR0 = 12; // this sets baud rate at 9600
	UCSR0A = (1 << U2X0); // this doubles data rate in asynchronous mode
	UCSR0B = (1 << TXEN0); // This enables transmit
	UCSR0C = (1 << UCSZ01) | (1 << UCSZ00); // This sets character size to 8 bits    
}

Now your main() starts to looks like:

int main(void)
{
    DDRB = 0xff;

    ADC_init();

    UART_init();

    while (1)
    {

which gives a much clearer "overall picture" of the structure and what's going on here. Similarly:

			  {
				  PORTB = (1 << Led_number); // This shifts a 1 along the register
				  ADCSRA |= 1 << ADSC; // This Starts Conversion
				  delay_ms (Delay + ADCH); // This adds Result of Conversion (ADCH) and calls delay function, as "_delay_ms()" requires a constant
				  Led_number = (Led_number + 1);
			  }
...
			  {
				  PORTB = (1 << Led_number); // This shifts a 1 along the register in the opposite direction
				  ADCSRA |= 1 << ADSC; // This Starts Conversion
				  delay_ms (Delay + ADCH);
				  Led_number = (Led_number - 1);
			  }

these two segments look almost identical apart from the +1 / -1 applied to Led_number at the end so you could easily make a single function for this stuff and just pass in the +1 / =-1 as a parameter:

void readAdc_setLed(int8_t increment)  {
	  PORTB = (1 << Led_number); // This shifts a 1 along the register
	  ADCSRA |= 1 << ADSC; // This Starts Conversion
	  delay_ms (Delay + ADCH); // This adds Result of Conversion (ADCH) and calls delay function, as "_delay_ms()" requires a constant
	  Led_number = (Led_number + increment);
}

But as already noted - the ADC reading is not being done right here. So that itself should be separated into a separate:

uint8_t readADC(void) {
    ADCSRA |= (1 << ADSC);
    while(ADCSRA & (1 << ADSC));
    return ADCH;
}

so the code is more like:

void readAdc_setLed(int8_t increment)  {
      uint8_t reading;
      
	  PORTB = (1 << Led_number); // This shifts a 1 along the register
	  reading = readADC();
	  delay_ms (Delay + reading); // This adds Result of Conversion (ADCH) and calls delay function, as "_delay_ms()" requires a constant
	  Led_number = (Led_number + increment);
}

But all the time keep thinking like this - consider when small/manageable sections of code can be split off into a function. This is especially true when a largely similar piece of code is repeated more than once.

Not quite sure what you mean by that?

 

The source code has to be written to the files on disk, as that's where the compiler will read from to do the compilation!

So, if it didn't save, the compiler would still be compiling the old file contents!

 

The usual way to save "history" is to use a version control system, which lets you retain the history of edits to the project.

 

One simplistic way to do this is to simply take a copy of the entire project tree each time you reach a point worth saving.

On the command line, you can use xcopy; or, in Windows Explorer, you can just drag & drop - and it will create a numbered copy.

Some tool chains have it in different orders:

i.e. Jumpstart C for AVR by ImageCraft

so it will depend on the toolchain used.....   so reference the doc

 

Jim

 

Or, perhaps, utoa(): 

 

https://www.avrfreaks.net/forum/itoa-question

 

you might want to explore a revision control system like SVN or Git. It makes "trying out ideas" then "winding back" trivial and what's more you get to keep all your experiments so if you later think "if only I'd known this when I was trying X, Y or Z it would probably have worked" then you can dig X/Y/Z out if the history, merge it onto what you've got  now and carry on the experiment.

	for ( i = 0; i <= 2; i++)
	{
		while (!(UCSR0A & (1 << UDRE0))); // This will loop until UDR0 is ready to handle data
		itoa(ADCH, pot, 10); // This converts the pot value in the ADC to a character
		UDR0 = pot[i]; // This places each character of the pot value on the serial line one byte at a time
	}

This is just plain weird. Why are you doing the itoa() inside the loop that is then consuming the converted characters ?? Surely you do:

    itoa(ADCH, pot, 10); // This converts the pot value in the ADC to a character
    for ( i = 0; i <= 2; i++)
    {
        while (!(UCSR0A & (1 << UDRE0))); // This will loop until UDR0 is ready to handle data
        UDR0 = pot[i]; // This places each character of the pot value on the serial line one byte at a time
    }

except that you wouldn't do this (see Kartiman's comments in #25) so it would look more like:

    itoa(ADCH, pot, 10); // This converts the pot value in the ADC to a character
    for ( i = 0; i <= 2; i++)
    {
        uart_putchar(pot[i]); // This places each character of the pot value on the serial line one byte at a time
    }

except you wouldn't even do that as you will almost certainly have a uart_putstring() to match uart_putchar() so it would actually be:

    itoa(ADCH, pot, 10); // This converts the pot value in the ADC to a character
    uart_putstring(pot);

Except that, as Kartman says, the real solution is FDEV_SETUP_STREAM so this all becomes:

printf("%d\r\n",ADCH);

 

+1

 

eg, see: https://www.avrfreaks.net/commen...

 

 

andy.brown100@yahoo.com wrote:

 simply pass a 13,

Avoid using "Magic Numbers" like that.

 

The 'C' programming language gives you '\r' for CR and '\n' for LF (newline).

 

 

 

This still looks wrong - you appear to be using ADCh (the reading) immediately after setting the start conversion bit. There's nothing here to wait for that conversion to complete before using ADCH - so how do you know it is valid?

 

Also:

			itoa(ADCH, pot_value, 10); // This converts the pot value in the ADC to a character
			for (int i = 0; i <= 2; i++)
			{
				serial_send (pot_value[i]);
			}

Again you don't seem to have grasped what we've been suggesting. The plan was that in addition to:

void serial_send(char val) // This converts the pot reading to a "char" and sends it to serial monitor
{
	while (!(UCSR0A & (1 << UDRE0))); // This will loop until UDR0 is ready to handle data
	UDR0 = val ; // This places a character on the serial line one byte at a time
}

you would then have:

void serial_send_string(char * str)
{
    while(*str) {
        serial_send(*str++);
    }
}

Now you can use:

            itoa(ADCH, pot_value, 10); // This converts the pot value in the ADC to a character
            strcat(pot_value, ' ');
            serial_send_string(pot_value);

You don't need this stuff any more:

			for (int i = 0; i <= 2; i++)
			{
				serial_send (pot_value[i]);
			}
			serial_send (' ');

(Anyway Kartman already told you that you can't use a fixed number in a for() loop anyway, suppose the ADCH reading were 0 or 3 or 17 or 91 or 137 or 255. If the reading were 3 for example (so "3" after the itoa) how would the "i <= 2" bit get on do you think?)

 

Oh and if you wonder what all the whil(*str) is all about - just read Freaks, it's been explained about a million times.

His code is not attempting to send either '\n' nor 'r' in fact. Just a space after the reading. So I rather think the diagonal is more like:

107 213 119 99  146
 241 101 205 182 13
2 155 135 222 207 1
44 etc.

which suggests it's simply that the width of each reading is not an exact divisor of the terminal width.

Nothing in the code in #29 is sending '\n' or '\r' ??

I trust you mean LINE feed? A Form feed is a whole new kettle of worms !

 

Bottom line: some receivers will take just "\n" to move to the start of the next line, some need "\r\n". Often it's a Windows versus Linux thing. (there may be some esoteric systems/terminals that require something other than "\n" or "\r\n" but they will be very few and far between)

 

Personally (probably because I spent so much of my life in a Linux world) I only ever send "\n". if the receiver does not then behave as I hope I use its own settings to make it do so (most terminals have definable action for \n and \r)

Windows uses explicit CRLF - which is probably why I tend to use both!

 

if the receiver does not then behave as I hope I use its own settings to make it do so (most terminals have definable action for \n and \r)

Indeed:

See #28, which links to https://www.avrfreaks.net/commen...

 

 

I guess using just LF is "safer", as it's easier to have the terminal implicitly add the line-feed if required than it would be to discard an unnecessary CR ... ?

You've missed the point!

 

The behaviour is configurable - so it doesn't depend on what application you have; it depends on how you have it configured!

 

Both PuTTY and TeraTerm can be configured: