Why is my I2C not read correctly?

We recommend not to use Serial functions in a interrupt routine.
Because the onReceive handler is called from a interrupt and the Serial functions use interrupts themself. Store the data in a global variable and process the data in the loop().

There is one special condition that is guaranteed to crash the sketch. That is when so many data is put into the Serial buffer (it is 64 bytes, inside the Serial library) that it becomes full. The Serial.print() will then wait until a new free spot comes available in that buffer. That buffer is emptied in a interrupt. If you do all of that from an interrupt, then it will not work. That is what you do.

Thanks, I didn't realize that would be a conflict. I modified my code to read the buffer every second in the loop, and I removed all of the Serial.print from the interrupt hook.

This appears to have fixed it.

We use a 'volatile' flag to let the loop() know that there is new data.

volatile byte data[25];
volatile bool newData;

void receiveEvent(int howMany) 
{
  if( howMany == 25)    // extra check must be done before using Wire.readBytes
  {
    // The Wire.readBytes is not the perfect function for this.
    // It can be used here if it is 100% certain that all the 25 bytes are available.
    Wire.readBytes( data, 25);
    newData = true;      // set flag to notify the loop() that there is new data.
  }
}

void loop()
{
  if( newData)    // is new data received ?
  {
    // process the data
    for( auto a:data)
    {
      Serial.println( a, HEX);
    }
    newData = false;     // release the data[] variable
  }
}

volatile is a good suggestion, but I didn't see that in any of the Wire.h examples. Notably, in an example from the library for a digital pot, the byte val = 0 is not declared volatile. Is there some documentation you could point me to that explains what is going on in the back-end? Specifically, if I was a newbie, what would I need to read to know that Serial functions inside interrupts are a bad idea?

One of those things you learn with experience, by reading tutorials on the web or in books, or on the forum. You may not call any function that uses interrupts, from inside an interrupt function, because interrupts are turned off temporarily.

Furthermore, in the main program, it is sometime necessary to turn off interrupts so that time-critical steps are not affected. There are functions for that purpose:

void loop() {
...
noInterrupts();  //turn off all interrupts
time_critical_action(); //do something
interrupts(); //back on
...

When the Arduino is a Master, it does not use interrupts. The Wire library uses internally interrupts, but that is hidden from the sketch.

When the Arduino is a Slave, then the onReceive and onRequest handlers are running in a interrupts. That is when the trouble begins :face_with_raised_eyebrow:

The 'volatile' keyword should be used when the loop() processes data that can be changed in a interrupt. With 'volatile' the compiler uses the data from the memory location of the variable, and without it, the compiler can optimize things by keeping the data in a register without looking at the variable.

There are many bad examples for using a Arduino as a Slave. Even the examples on the official Arduino reference pages.

Serial functions inside a interrupt is a bad idea. It is. Everyone can confirm that.
The addition by @jremington about disabling interrupts is of course correct. If you need to keep the 25 bytes together, then you might have to make a copy while the interrupts are turned off.

Basic tutorial by Nick Gammon: http://www.gammon.com.au/i2c

My wiki at Github: https://github.com/Koepel/How-to-use-the-Arduino-Wire-library/wiki.
If you read all the pages, then you are a I2C expert :wink:

Tutorial by Robin2: https://forum.arduino.cc/t/use-i2c-for-communication-between-arduinos/653958
It show how to transfer data over I2C. A 'struct' is the easiest way.

Tutorial by GolamMostafa: https://forum.arduino.cc/t/ch-6-i2c-bus-based-serial-data-communication/659019
I'm not sure if this is the Master-Slave example that I was looking for.

The noInterrupts() command is interesting. I've never used that one. The usage is reminiscent of the semaphore.h commands used for multithreading.

Thanks for the excellent list of resources. I glanced at Nick Gammon's writeup prior to starting this post, but I must have missed the bold, obvious notes which summarize what went wrong here:

You should not :

  • Do serial prints
  • Use "delay"
  • Do anything lengthy
  • Do anything that requires interrupts to be active

I am attempting to communicate between a 3.3V custom Mega2560 and a 5V Nano using Wire I2C. In a previous question, helpful folks encouraged me to tweak resistors and not make stupid Serial interrupt mistakes, but I am having new problems.

Why does my controller unit not read any bytes when it requests them?

Simplified Code:

Master/Controller

#define I2C_FASTMODE 1
#define I2C_TIMEOUT 1000
#define I2C_PULLUP 0
#define SDA_PORT PORTC
#define SDA_PIN 3 // = 34 on mega
#define SCL_PORT PORTC
#define SCL_PIN 5 // = 32 on mega
#include <SoftWire.h>

SoftWire soWire = SoftWire();
struct CMD_DATA_STRUCTURE {
	volatile byte msg[25];
};
CMD_DATA_STRUCTURE cmddata;

void xon_cmd(int unit) {
	for (size_t i = 0; i < 25; i++) {
		cmddata.msg[i] = 0x00;
	}
	// Add SOH, address, NACK, and EOT
	cmddata.msg[0] = 0x01; // SOH
	cmddata.msg[1] = 0x30 + unit; // ASCII "0" is master node
	cmddata.msg[2] = 0x11; // XON
	cmddata.msg[24] = 0x04; // EOT
	return;
}

void issue_cmd() {	
	soWire.beginTransmission(8);
	for (size_t i=0; i < 25; i++) {
		soWire.write(cmddata.msg[i]);
	}
	soWire.endTransmission(false);
	Serial.println("");
}

void setup() {
	if (DEBUG) {
		// Init serial comms for debugging
		Serial.begin(9600);
		Serial.print("Debugging: ");
		Serial.println(__FILE__);
		}
		Serial.println("");
	}
	
	soWire.begin();
}

void loop() {
	xon_cmd(2);
	issue_cmd();
	delay(100);
	soWire.requestFrom(8, 25);
	while(soWire.available()) {
		byte c = soWire.read();
		Serial.print(c, HEX);
	}
	delay(1000);
}

Slave/Peripheral

#include <Wire.h>
#include "SoftEasyTransfer.h"
int HAVE_MSG = 0;
int HAVE_REQ = 0;
struct CMD_DATA_STRUCTURE {
	volatile byte msg[25];
};
CMD_DATA_STRUCTURE cmddata;

void ack_cmd() {
	for (size_t i = 0; i < 25; i++) {
		cmddata.msg[i] = 0x00;
	}
	// Add SOH, address, ACK, and EOT
	cmddata.msg[0] = 0x01; // SOH
	cmddata.msg[1] = 0x30;
	cmddata.msg[2] = 0x06; // ACK
	cmddata.msg[24] = 0x04; // EOT
	return;
}

void receiveEvent() {
	int cnt = 0;
	while (Wire.available()) {
		cmddata.msg[cnt] = Wire.read();
		cnt++;
	}
	HAVE_REQ++;
	return;
}

void requestEvent() {
	ack_cmd();
	HAVE_MSG++;
	Wire.write((byte *) &cmddata, sizeof(cmddata));
	return;
}

void setup() {
	if (DEBUG) {
		// Init serial comms for debugging
		Serial.begin(9600);
		Serial.print("Debugging: ");
		Serial.println(__FILE__);
	}

	Wire.begin(8);
	Wire.onReceive(receiveEvent);
	Wire.onRequest(requestEvent); // register event
}

void loop() {
	Serial.print(HAVE_MSG);
	Serial.print(" ");
	Serial.print(HAVE_REQ);
	Serial.print(" ");
}

Expected Behavior

When I run this and monitor the command unit, I should see bytes received from the peripheral. When I monitor the peripheral unit, I should see the HAVE_MSG and HAVE_REQ counters increase roughly every second

Observed Behavior

The counters increase correctly on the peripheral, but nothing is reported by the command unit.

Notes

  • This code is attempting to communicate using the default Wire and SoftI2CMaster's SoftWire wrapper. The soft variant has no methods for peripheral units by design, but there should be nothing stopping us from kludging together a peripheral device using Wire...right?
  • I tried switching from I2C protocol to Serial/SoftSerial solution, but I observed similar results. The command unit cannot read info coming to it from the comm lines.
  • I don't have a good logic analyzer on hand, but I do have a spare Arduino. The signal is going faster than my little Uno can print conveniently, but I can tell that the signals are moving.

Do you use SoftWire by Steve Marple from the Library Manager ?

When connecting a 3.3V I2C bus to a 5V I2C bus, there is a voltage mismatch and you might expect troubles.
On top of that, you use a software I2C library, a sketch that is too complex, you create a full Serial TX buffer in the Slave (which is bad), there is no STOP condition in the function issue_cmd(), some variables should be 'volatile', using a 16-bit integer on a 8-bit microcontroller in a interrupt is not fail-safe, and so on.
I see at least six problem at first glance.

Can you start with two Arduino Uno boards and transfer a single byte ? If that works, then you can try SoftWire on the Master board, if that works, then you can try to transfer two bytes, and so on.

Why do you use a software I2C library ? Is there a good reason for that ?

Did you connect the grounds ?

Did you read my opinion that the I2C bus is not the best solution between Arduino boards and that a voltage mismatch on the I2C bus can not be ignored and Arduino in Slave mode requires knowledge about clean programming ? https://github.com/Koepel/How-to-use-the-Arduino-Wire-library/wiki/How-to-make-a-reliable-I2C-bus

[ADDED] I am very fond of the LHT00SU1 logic analyzer in combination with Sigrok/PulseView. It can decode Serial/UART and I2C data. https://www.banggood.com/Geekcreit-LHT00SU1-Virtual-Oscilloscope-Logic-Analyzer-I2C-SPI-CAN-Uart-p-988565.html

@whoneyc:

We strongly discourage starting new posts that continue a topic. In your previous post, you stated that you were using the required level shifter for the 5V to 3.3V I2C connection.

Here, that omission misled forum user Koepel and potentially creates a lot of confusion.

Please ask the moderator to merge these two threads. (Flag Icon).

Not entirely. As you can see I did not give a working sketch. I wrote that there is a truck load of problems.

I'm very sorry about causing any confusion. I was working from the assumption that continued questions in a thread would be inappropriate (a la Stack Overflow), so I should make a new one when the thrust of the question had changed. I will flag and request the merge.

There is no need to merge.
This is not a strict "Question - Answer" forum. This forum is to help with Arduino in every way.

Can you try to make the Serial/UART communication work ?

I'm using the term "Serial/UART" for a serial signal with RX and TX and a baudrate, because some call the I2C bus also a "serial" bus.

Thanks for listing out the problems you see. I hope I am able to clarify some things.

Do you use SoftWire by Steve Marple from the Library Manager ?

No. I was using SoftI2CMaster with the Wire wrapper by Bernhard Nebel and Peter Fleury. I chose this one because the previous operation of the Command board used it to communicate with a sensor. I can take a look at Steve Marple's library and see if that behaves better or is easier to debug.

When connecting a 3.3V I2C bus to a 5V I2C bus, there is a voltage mismatch and you might expect troubles.

Correct, but there is a hardware level shifter. I have a PCA9517 between the different voltage devices. Currently, there are 4.7k pull ups on the 5V side and 3.3k pull ups on the 3.3V side. I apologize for breaking this into a new forum topic (which is apparently frowned upon at this forum), as the level shifting and resistors are discussed in the original thread.

While level shifters are not ideal (as you note in your write-up), this setup has been used before to communicate between the Command board and a 5V sensor board.

you use a software I2C library
Why do you use a software I2C library ? Is there a good reason for that ?

Yes. I do not have access to the standard Arduino ports for I2C/Serial comms on the Command board. Using a default Arduino is not appropriate to the application of the project for various reasons not worth discussing here.

a sketch that is too complex

you create a full Serial TX buffer in the Slave (which is bad)

What is bad about this? Can you provide more info? Should I not be using a struct/buffer to hold the data which are communicated between the two units?

there is no STOP condition in the function issue_cmd()

I'm confused by this. This is not a requestFrom command, but sending a package to the peripheral. It is terminated by endTransmission.

some variables should be 'volatile'
using a 16-bit integer on a 8-bit microcontroller in a interrupt is not fail-safe

Are we talking about the HAVE_MSG and HAVE_REQ counters? Those are only there for quick debugging/demonstration, but they can be made volatile uint8_t easily enough.

Did you connect the grounds ?

Yes.

Bonus info: the SCL and SDA are separated by a ground wire, not next to each other.

Did you read my opinion that the I2C bus is not the best solution between Arduino boards and that a voltage mismatch on the I2C bus can not be ignored and Arduino in Slave mode requires knowledge about clean programming ?

Of course. The voltage mismatch would be something I'd love to ignore, but I know I can't! The "clean programming" is a challenge and kinda vague. I am hacking my way through this and learning as I go. Learning what counts as "clean" has to come from "dirty" failure.

I am very fond of the LHT00SU1 logic analyzer in combination with Sigrok/PulseView. It can decode Serial/UART and I2C data.

Thanks for the suggestion. That seems very similar to what I used in my last lab, but I don't have one at my home lab.

Can you try to make the Serial/UART communication work ?

I'm using the term "Serial/UART" for a serial signal with RX and TX and a baudrate, because some call the I2C bus also a "serial" bus.

I tried that prior to posting my question. As I put in the "Notes" section, the Serial on the command board was unable to read the output from the peripheral.

Also note, that the PCA9517 level shifter used on the command board is "designed" for I2C, not UART. It likely is compatible, but this was why I wanted to try I2C first.

If Serial/UART does not work, then making it ten times harder by trying the I2C bus might not be a good solution.

felias-fog has a slow version with is more compatible. I think you better us that one to start with.

Can you strip your sketch from everything that is not needed ?

Make an array to transfer:

byte myData[4] = {0xAA, 0x55, 0x40, 0xFE};  // just a few different numbers

This will fill the TX buffer (inside the Serial library):

void loop() {
	Serial.print(HAVE_MSG);
	Serial.print(" ");
	Serial.print(HAVE_REQ);
	Serial.print(" ");
}

Make the baudrate 115200 and put a delay() in the loop(), for example 100 ms.

I told you have to pass the data from the onReceive handler to the loop(): Why is my I2C not read correctly? - #11 by Koepel

If you do something else, you have to prove to us that it works :wink:

How far do you think you are until the I2C communication works ? I guess you are at 5%.
Arduino provides the possibility to turn a Arduino board into a I2C Slave, but there is no official way to do that in a easy way.

Topics merged at the request of @whoneyc.
Regards, Per

I was testing a Serial version of the code (see below), and I discovered something that may have been exacerbating problems: the ground wire had disconnected between the units. Granted, my i2C code had problems, but at least now I know why the Serial was not working before.

This works on a Uno+Nano but not on Command+Nano

Code that is currently working:

Command

#include <SoftwareSerial.h>

SoftwareSerial mySerial(32, 34); // RX, TX

struct CMD_BYTE_BUFFER {
	byte msg[4] = {0x40, 0x41, 0x42, 0x42};
};
CMD_BYTE_BUFFER cmddata;

void setup() {
	// Power up 5v lines
	pinMode(A1, OUTPUT);
	pinMode(A7, OUTPUT);
	digitalWrite(A1, HIGH);
	digitalWrite(A7, HIGH);
	
	Serial.begin(9600);
	Serial.print("Debugging: ");
	Serial.println(__FILE__);
	for (size_t i; i<4; i++) {
		Serial.print(cmddata.msg[i]);
		Serial.print(" ");
	}
	Serial.println();

	mySerial.begin(9600);
	mySerial.flush();
}

void loop() {
	for (size_t i=0; i<4; i++) {
		mySerial.write(cmddata.msg[i]);
	}
	mySerial.write(0x04);
	for (size_t i=0; i<4; i++) {
		cmddata.msg[i]++;
	}
	delay(1000);
}

Peripheral

#include <SoftwareSerial.h>

SoftwareSerial mySerial(A4, A5); // RX, TX

struct CMD_BYTE_BUFFER {
	byte msg[4] = {0xFF, 0xFF, 0xFF, 0xFF};
};
CMD_BYTE_BUFFER cmddata;

size_t CNT = 0;


void setup() {
	Serial.begin(9600);
	Serial.print("Debugging: ");
	Serial.println(__FILE__);
	for (size_t i; i<4; i++) {
		Serial.print(cmddata.msg[i]);
		Serial.print(" ");
	}
	Serial.println();
	
	mySerial.begin(9600);
	mySerial.flush();
}

void loop() {
	if (mySerial.available()) {
		byte temp = mySerial.read();
		if (temp == 0x04) {
			Serial.println();
			CNT = 0;
			return;
		}
		cmddata.msg[CNT] = temp;
		Serial.print(cmddata.msg[CNT], HEX);
		CNT++;
		Serial.print(" ");
	}
} 

Follow-up

While it may not have been obvious in the previous post...that code did NOT work on my custom 3.3V command board, only on a standard 5V Uno. I spent the weekend playing with resistors on the level shifter to no avail. While it worked fine to interface with 5V sensors, the level shifter I had did NOT want to play nice for Arduino-Arduino communication.

The Real Solution

Since my custom 3.3V command board is fairly locked in, I decided to NOT use it. Sure, I could hand wire a different IC onto the level shifter pads, but that would add more complexity to the of the circuit and introduce new challenges.

Instead, I am starting from scratch with an Arduino Mega and one of Lady Ada's lovely datalogging shields. While I am concerned about how this limits many of the features which I wanted from the custom board (lots of high stability power lines, voltage monitoring chips, buttons, etc.) it would be easier to build these from scratch or forget them entirely.

TL;DR

Don't bother trying to create an Arduino peripheral/slave with a voltage difference between the peripheral and command board. It's a pain in the tuchus.