Atmega 328p RTC DS1307 problem

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

Quote:
Hello everyone, I am working with the DS1307 rtc chip. I have written a simple and a must working easily understandable program interfaced with the at328p MCU. The i2c_start() has a clear ack of 08 on the serial port tested on terminal. But when i2c_send(uint8_t addr) is executed, the status code returned back is 00 which is very very uncertain. I have clearly explained my problem, if anybody has a doubt ready to explain once again...but kindly help out..
I have provided my working code as well.
Please help out !!!!!!!!!!!!

#define F_CPU 1000000UL		//	1MHz
//#define F_CPU 8000000UL
#define F_SCL 50000UL		//	50kHz

/*-------------------------------------------*/

#include
#include

//	Definitions for all states of i2c in 168

//		7	 6	 5	    4	 3	  2	   1   0
//	|TWINT|TWEA|TWSTA|TWSTO|TWWC|TWEN|---|TWIE

#define link_start	0xA4
#define link_send	0x84
#define link_stop	0x94
#define link_ACK	0xC4
#define link_NACK	0x84
//#define link_ready	(TWCR & (1<<TWINT))
//#define link_status	(TWSR & 0xF8)

//	Definitions for all registers in RTC

#define DS1307 	0xD0

#define seconds 0x00
#define minutes 0x01
#define hours	0x02

//	function declarations

void mcu_init(void);
void i2c_init(void);
void ser_init(void);

void usart_tx(uint8_t);

void i2c_write_time(uint8_t, uint8_t, uint8_t);
void date_time(void);
void time(void);

uint8_t read_time(uint8_t, uint8_t);

//	function declarations for i2c operations

uint8_t i2c_start(void);
uint8_t i2c_send(uint8_t);
void i2c_stop(void);
uint8_t i2c_write(uint8_t);
uint8_t i2c_NACK(void);

uint8_t link_status;

int main(void)
{
	ser_init();
	mcu_init();
	i2c_init();

	usart_tx(0xCC);

//	set default time 00:00:00 -- time being

	i2c_write_time(DS1307, seconds, 00);
//	usart_tx(0x00);

	i2c_write_time(DS1307, minutes, 00);
//	usart_tx(0x00);
	
	i2c_write_time(DS1307, hours,	00);
//	usart_tx(0x00);

	while(1)
	{
		date_time();
		_delay_ms(1000);
		usart_tx(0xAA);
		usart_tx(0xDD);
	}	
}

void mcu_init(void)
{
	DDRB = 0xFF;
	DDRC = 0x00;
}

void i2c_init(void)
{
	TWSR = 0;
	TWBR = (((F_CPU/F_SCL)-16)/2);
	TWCR = (1<<TWEN);
	usart_tx(TWBR);
}

void ser_init(void)
{
	UCSR0B=0x18;	//	enable transmitter and receiver
	UCSR0C=0x36;	//	8DATA BBITS 1 STOP BIT AND ASYNC MODE

	UBRR0=0x000C;	//	4800bps for 1MHz
//	UBRR0=0x0067;	//	4800bps	for 8MHz
	UCSR0A=0x02;	//	DOUBLE SPEED
}

void usart_tx(uint8_t data)
{
	while(!(UCSR0A & (1<<UDRE0)));
	UDR0 = data;

	while(!(UCSR0A & (1<<TXC0)));
	UCSR0A |= (1<<TXC0);
}

void i2c_write_time(uint8_t dev_addr, uint8_t dev_regs, uint8_t dev_data)
{
	i2c_start();
	i2c_send(dev_addr);
	i2c_write(dev_regs);	//	set register to be written
	i2c_write(dev_data);	//	write the register
	i2c_stop();
}

void date_time(void)
{
	time();
}

void time(void)
{
	uint8_t screen;

	screen = read_time(DS1307,hours);
	usart_tx(screen);
	usart_tx(':');

	screen = read_time(DS1307,minutes);
	usart_tx(screen);
	usart_tx(':');

	screen = read_time(DS1307,seconds);
	usart_tx(screen);

	usart_tx(0x0D);	//	CR
	usart_tx(0x0A);	//	LF
}

uint8_t read_time(uint8_t dev_addr, uint8_t dev_regs)
{
	uint8_t time_val = 0;
	
	i2c_start();
	i2c_send(dev_addr);
	i2c_write(dev_regs);

	i2c_start();	//	re - start
	i2c_send(dev_addr | 0x01);	//	set in read mode

	time_val = i2c_NACK();
	i2c_stop();

	return time_val;
}

uint8_t i2c_start(void)
{

	TWCR = link_start;
	while(!(TWCR & (1<<TWINT)));

	usart_tx(0x01);
	link_status = (TWSR & 0xF8);
	usart_tx(link_status);

	return ( link_status==0x08 );
}

uint8_t i2c_send(uint8_t rtc_addr)
{
//	usart_tx(rtc_addr);

	TWDR = rtc_addr;
	TWCR = link_send;
	while(!(TWCR & (1<<TWINT)));

	link_status = (TWSR & 0xF8);
	usart_tx(0x02);
	usart_tx(link_status);

	while(link_status==0){	link_status = (TWSR & 0xF8);}
	usart_tx(link_status);
	return( link_status == 0x18 );
}

uint8_t i2c_write(uint8_t rtc_regs)
{
	TWDR = rtc_regs;
	TWCR = link_send;
	while(!(TWCR & (1<<TWINT)));
	
	usart_tx(0x03);
	
	link_status = (TWSR & 0xF8);
	usart_tx(link_status);

	if(link_status != 0x28)
		return 1;
	else
		return 0;
}

uint8_t i2c_NACK(void)
{
	TWCR = link_NACK;
	while(!(TWCR & (1<<TWINT)));
	usart_tx(0x04);

	link_status = (TWSR & 0xF8);
	usart_tx(link_status);

	return TWDR;
}

void i2c_stop(void)
{
	TWCR = link_stop;
	usart_tx(0x05);
}

:) :) :) :) :)

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

Why on earth do you think that your question needs a vote? What if 100% of votes are "No". Will you rush out and cut your wrists? Be sensible... what if only idiots vote... would that influence whatever you are seeking?

Ross McKenzie ValuSoft Melbourne Australia

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

Do you want a poll or help with your code?

Your coding style is unconventional. #defines are upper case by convention.

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

Also if you are posting code for review why would you leave some dead/commented lines in it?

What I do if there's some commented stuff I think I might want to use later is to commit a copy to my revision control system with the commented stuff. Then tidy up and commit/use the "clean" copy. If I never need to get back to the stuff that wasn't being used I can just access the version that had it (the joy of code control!).

BTW a line such as this is a bit "anonymous":

   link_status = (TWSR & 0xF8);

I wonder what is in those 3 bits you are isolating? I'd replace the 0xF8 with something more documentary.

Also white space costs nothing. It's kind of unconventional not to have a space before the angle bracket in:

#include
#include

Personally I also adhere to a C coding standard that mandates space on either side of every C operator and that includes "<<" so:

   TWCR = (1<<TWEN);
   while(!(UCSR0A & (1<<TXC0)));
   UCSR0A |= (1<<TXC0);

would be:

   TWCR = (1 << TWEN);
   while(!(UCSR0A & (1 << TXC0)));
   UCSR0A |= (1 << TXC0);

etc.

In this:

void time(void)
{
   uint8_t screen;

   screen = read_time(DS1307,hours);
   usart_tx(screen);
   usart_tx(':');

   screen = read_time(DS1307,minutes);
   usart_tx(screen);
   usart_tx(':');

   screen = read_time(DS1307,seconds);
   usart_tx(screen);

it may not be obvious to you but you'd get exactly the same code generated if you used:

void time(void)
{
   uint8_t hrs, mins, secs;

   hrs = read_time(DS1307,hours);
   usart_tx(hrs);
   usart_tx(':');

   mins = read_time(DS1307,minutes);
   usart_tx(mins);
   usart_tx(':');

   secs = read_time(DS1307,seconds);
   usart_tx(secs);

the name "screen" means nothing. In fact you might as well just use:

void time(void)
{
   usart_tx(read_time(DS1307,hours));
   usart_tx(':');

   usart_tx(read_time(DS1307,minutes));
   usart_tx(':');

   usart_tx(read_time(DS1307,seconds));

which, again, is almost bound to generate identical code (screen or hrs,mins,secs will all just be R24). The only reason to read to the intermediate variable might be if you want to breakpoint the code and see what's been read before output.

I don't like this line:

   while(link_status==0){   link_status = (TWSR & 0xF8);}

again white space (vertical or horizontal) costs nothing so this would be much clearer as:

   while (link_status == 0) {   
     link_status = (TWSR & 0xF8);
   }

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

Quote:
Thanks for ur points clawson. I would keep them in mind and ur reply as a record But b4 all this, can you look into the issue caused by the function i2c_send(). I don't know why Status code returned is 00 when seen on terminal. 'screen' was as a reference variable which is being printed on the monitor. And oxF8, the remaining three bits are the prescaler which I am not required to see it often. Refer TWSR register in the datasheet.

I see no point in asking votes Valusoft. Anyways ur help ll be more appreciated than ur critics :D i can have n numbers of criticism for askin votes, but I need ur help ppl....

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

Quote:

ok kartman, please help me out. the function causing problem is i2c_send(addr) in i2c_write_time()

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

I'd be lazy and look at the arduino code.

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

I2C is fairly easy to use, but does have a few rules that need to be followed. Device addresses are the upper 7 bits with the LSB indicating to the device if the operation is a read or write, one way of doing this to define two addresses for the device, ie. 0xD0 for a write operation, and 0xD1 for a read operation, I may have missed it, since you only have one address defined, where you mask in the R/W bit when sending the address to the device.
One more note, when reading data from an I2C device, you must ACK each data byte except the last one and it must be NAK so the master can perform a stop or the bus hangs.

JC

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

Quote:

But b4 all this, can you look into the issue caused by the function i2c_send(). I don't know why Status code returned is 00 when seen on terminal.

I've never understood I2C and I don't plan to try and understand it now so, no, I'm afraid I can't help - but I thought this thread was about asking for a peer review of your code - not about fixing some fault?

In the unlikely event someone made me use I2C then I wouldn't try to write it myself. I'd use the tried and tested I2C code for avr-gcc from Peter Fleury.

Quote:

And oxF8, the remaining three bits are the prescaler which I am not required to see it often.

Yes, but me, the reader of the code - the possible future "maintainer" or you when you come back to this code in 18 months don't know that the bottom 3 bits are the prescaler.
Quote:

Refer TWSR register in the datasheet.

yeah but why make it difficult for the maintainer. Try to make the code as self-documenting as possible. Either a simple comment "bottom 3 bits are prescaler and not needed" or just write the line as:

 link_status = (TWSR & ~((1 << TWPS1) | (1 << TWPS0))); 

granted the maintainer may not know what TWPSn are but they could probably have a pretty good guess that "PS" are prescaler bits and it means more to them than "0xF8" which says nothing apart from "bottom 3 bits".

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

I never understand why people find I2C difficult.

You download a respected library, read the documentation, use every return value when available.

Of course, your pride may be to big.
In which case, you use the proven library but say it was all your own work.

If you can make improvements to Stang, Fleury, Atmel, ... by all means do so. Then publish them for the benefit of the world.

David.

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

Quote:

I never understand why people find I2C difficult.

I guess it's maybe that my first experience was implementing bit-bang I2C on a CPU that didn't have it in hardware for a DECT cordless phone design. It was a bit of a dog and not something I'd want to repeat but perhaps with a real I2C peripheral it's plain sailing?

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

Actually Cliff, it is pretty easy to bit-bang. Just look at some 8051 code.

Of course, I2C like most buses uses GPIO pins in 'open-drain' mode. i.e. this is the default with 8051 MCU.

But is perfectly simple to implement with the DDRx of an AVR or the TRISx of a M*crochip. And even a 8025 port expander can change an individual port pin direction.

The important thing to remember is that any bus can go wrong. So you always check ACK.

The 'reasons' for going wrong are given to you in the TWSR or other hardware status. But even a bit-bang will tell you if something is wrong. You have to do the detective work for why.

David.