[sdcc-devel] [ sdcc-Patches-3413443 ] PIC16: I2C, several bugs

[SourceForge.net <no...@so...>]

Patches item #3413443, was opened at 2011-09-23 23:10
Message generated for change (Comment added) made by diegoherranz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=3413443&group_id=599

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Closed
Resolution: Accepted
Priority: 5
Private: No
Submitted By: Diego Herranz (diegoherranz)
Assigned to: Nobody/Anonymous (nobody)
Summary: PIC16: I2C, several bugs

Initial Comment:
Several bugs found in PIC16 I2C library.

-#define I2C_SLEW_OFF	   0xc0
+#define I2C_SLEW_OFF     0x80
Slew rate control is only the 7th bit of SSPSTAT and 6th (SMbus) was also set.

- Macros semicolons in i2c.h changed to comma operators (http://en.wikipedia.org/wiki/C_preprocessor#Multiple_statements).

- I2C_IDLE() macro had semicolon at the end. Removed.

- i2c_write() now clears SSPCON1bits.WCOL before writing to SSPBUF.
If there was a write collision on a previous writing, you might think it happened at the current one. And it would also happen at the next ones.

- i2c_write() now does "I2C_IDLE()" instead of "while(I2C_DRDY())".
SSPSTATbits.BF is cleared by hardware after 8 bits have been output and SSPSTATbits.R_W is cleared by hardware after the ninth bit (ACK), at the same time that SSPIF would be set. So, I think that SSPSTATbits.R_W is the flag to check and I2C_IDLE() checks it. I2C_DRDY() does not.


Not it the patch: maybe, I2C_IDLE() should be taken out of i2c_write() so execution doesn't block if you use i2c interrupts. If you don't use interrupts, you would need to call I2C_IDLE() after calling i2c_write().

If there's any doubt please let me know.

Thank you very much

----------------------------------------------------------------------

Comment By: Diego Herranz (diegoherranz)
Date: 2011-10-16 16:37

Message:
I think that do{...}while(0) can also work. Anyway, I found it unnecessary
on single statement macros.

Thank you very much for your help.



----------------------------------------------------------------------

Comment By: Raphael Neider (tecodev)
Date: 2011-10-16 12:55

Message:
Fixed in r6971.

I find do { ... } while (0) more explicit than the comma-operator (easily
mistaken for a semicolon), so I changed the macros to use the loop syntax.
Should behave identically, though ... If not, please speak up.

As for taking I2C_IDLE out of i2_writec, I am hesitant to changing the
semantics of the library function from blocking to non-blocking. If
non-blocking write is required/requested, one could add i2c_writec_nb() or
something to the library.

Thank you for the report and the reminder.

----------------------------------------------------------------------

Comment By: Diego Herranz (diegoherranz)
Date: 2011-10-16 11:48

Message:
Any possibility to address this before 3.1.0?

Maybe I should have posted it in bugs and not in patches, I don't know. If
so, I'm sorry.

Thank you

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=3413443&group_id=599