Code for I2C Slave Peripherals

Projects Networking, Protocols, and Devices
1

Please review my organisational paradigm, for coding Arduino I2C slave peripherals.

https://green.bug-eyed.monster/arduino-i2c-slave/

Your welcome to leave comments, and I'd love to hear your feedback.

GBEM

2

In OnReceive() you forgot to check the number of bytes received.
In OnRequest() you try to read a register number that was not sent.

Please become more familiar with I2C coding before you broadcast such nonsense.

In case you got your crap from ChatGPT then you should have learned now that you can not replace own knowledge by Artificial Ignorance.

1 Like
3

Well, I posted a link to the code, knowing and hoping that it could be improved.

I would very much welcome your sharing your expertise, to help me make it the best code available.

I think I have a start on making the requisite changes, though I'm not 100% sure, and would appreciate if you were able to show me directly, DrDiettrich?

GBEM.

4

You pretended to help others with useful information and code. I suggest that you delete whatever material belongs to your topic and start over from the fine IDE examples.

Do you understand the difference between asking for assistance and claiming to have better knowledge than most others?

2 Likes
5

Yes, I do.

6

@green_bug-eyed_monster Some of us are for years on this forum. We see the same problems over and over again.

When you start using a Arduino board as a I2C Slave, then you have opened a can of worms.

What you publish is a start. However, if a beginner would follow it, then it might not even work in the end, because of the many problems.

I can mention a few:

  • You forgot to mention pullup resistors.
  • The Arduino Uno can be a reliable I2C Slave, I'm not sure about the other boards.
  • When connecting a 3.3V Raspberry Pi to a 5V Arduino board, then there is a mismatch in the voltage levels and the noise margin is gone.
  • There are libraries that turn off the interrupts. If those library are used in a Arduino Slave, then it interferes with the Arduino board responding to a Master.
  • There was/is a bug where some boards receive a message of zero bytes, followed by another call to receiveEvent() with the real data.
  • You have two bytes for the length, but the length is limited to 32 bytes for the basic Arduino boards.
  • and so on.

I can write pages about the I2C bus (which I did).

Have you seen the tutorial by Adafruit: https://learn.adafruit.com/working-with-i2c-devices/overview
I replied to that tutorial with a list of improvements, but they seem to ignore it.

I see that you come from the 8051 world. What that in mind, it makes perfect sense what you write. But the I2C bus can be very vague when used in the wrong way.

2 Likes
7

@Koepel,

Thanks you for a professionally toned, and very comprehensive take on I2C code and advice provisioning.

Yes, I've seen only a very few examples of what I set out to achieve. None were good. By the sounds, it was better that I did not find the adafruit tutorial you mentioned.

Here, the difference, that I propose an improvable solution. Unlike adafruit, read the messages attached to my article, and make edited upgrades.

I'm not the expert on I2C code for Arduino. Whilst my write-up is primarily focussed on a code organisation, I thought my supporting code was passable. Knowing that it might not be, I welcome every bit of improvement related advice.

Thanks again for a pleasantly toned response.

GBEM

8

In the Arduino world, extra abstraction layers are often only confusing. The main rule is to keep it simple.
I have started a few times to make a flexible software layer that can be used for any purpose. That didn't work out.

The most straightforward way to communicate over the I2C bus is to declare a struct in both the Slave and the Master, and transfer that struct.
Here is a tutorial: https://forum.arduino.cc/t/use-i2c-for-communication-between-arduinos/653958

Both onReceive and onRequest handlers are called from a interrupt. They should be as short as possible.
It is best to process the data in the loop(). The onReceive fills the global struct with data and sets a flag. In the loop(), the flag is check to see if there is new data.

What I write here has major consequences and can break a project.
For example: New data could arrive while the loop() is in the middle of reading the data from the struct. Then the code reads a few old bytes and a few new bytes. For that, extra flags can be added, or a copy of the struct can be made. As if it were a single element buffer protected by semaphores.
The onReceive handler could discard new data if the loop() has not reset the flag, but who keeps track of discarded data ?

1 Like
9

Hi Koepel,

Hmm...I've taken a quick look at the tutorial link you're kindly provided, and will study it further.

I'm not sure the structure approach is the best way to go, and prefer the array of 16-bit registers. On the 328PB I'm using, 16-bit reads and writes are atomic. You mentioned a weakness in the tutorial code, in that it can produce half-stored outgoing data in the task loop(). Well, aren't atomic registers a more robust alternative? Besides, code libraries, like wiringPi, that are used in master solutions, employ higher-level functions, for operations on 8-bit addressable, 8 or 16-bit registers. Dealing with the struct requires more complicated, low-level master code.

I'm also unsure I should use flags to indicate register changes. In the example, I found this only applicable to the case where data is externally consumed within an ISR, contemporaneously with a task loop() write. Here again, when dealing with 16-bit registers, the load and store operations are atomic. As sources can never produce half-written data, the change flags become redundant, and the code a great deal simpler.

I understand that the employment of structs may yield greater flexibility, though for someone reading a few sensors, and controlling a motor or two, the added complexity may be an unwarranted hindrance. I agree with your suggestion to "keep it simple", and have also incorporated abstraction for just that reason.

Thanks again for the very welcome insights into the methods suggested in your link.

Regards,
GBEM

11

An array with integers will make the code a little easier. But sooner or later something else is added and a struct is needed :wink:

The ATmega328PB is atomic for reading and writing 16 bit registers, such as the registers of timers. But as far as I know, reading and writing 16 bit data from and to memory is not atomic.

Suppose that someone uses an array of 4-byte float numbers. Then the code could read a floating point number with half the old value and half the new value.

I was not talking about a flag for register changes.
When the Arduino Wire library calls the onReceive handler (from within an interrupt routine), then the data is in a buffer inside the Wire library. When that data involves commands that will take some time, then it is better to let the code in the loop() process the data.
Therefor reading all the data, storing it in a global variable and telling the code in the loop() that new data has arrived is a good solution.

12

Hi Koepel,

I see your point about speeding onReceive, by buffering incoming data, and they setting a flag. Yes, I2C is forgiving in terms of timing, but only to a point. I did think it odd that both incoming and outgoing data required flags. Now I see why.

Please lemmy get back to you on the atomicity of 328PB loads and stores. I know that increments and decrements of 16-bit variables aren't atomic, but will have to check some assembly and an instruction manual to be sure of other 16-bit memory operations.

GBEM

13

To compare it with the ATmega328P, here is a sketch and a part of the code.

volatile uint16_t myData;

void setup() 
{
  myData = 0x55AA;
}

void loop() { }

 1aa:	8a ea       	ldi	r24, 0xAA	; 170
 1ac:	95 e5       	ldi	r25, 0x55	; 85
 1ae:	90 93 01 01 	sts	0x0101, r25	; 0x800101 <_edata+0x1>
 1b2:	80 93 00 01 	sts	0x0100, r24	; 0x800100 <_edata>
2 Likes
14

Bother. Well that's death for my present example.

Despite this being a common approach, I did check my facts. Admittedly, however, I chose to consult a known-to-be-incompetent source.

Thanks for going the extra mile for me, Koepel.

I wonder if I can have a thread expunged? No, wait. That's a very simple thing to fix. Gimme a bit... How's this ?

GBEM

15

Arrived data in the Slave must be collected in the onReceive() routine and saved in global variables; otherwsie, they will be lost. Before leaving the onReceive() routine, the Slave must set a global flag to inform the loop() function that new data are available in the global variables. Therefore, setting flag is not an odd/optional task; rather, it is a mandatory task.

void onReceive(int howMany)
{
    for(int i = 0; i < howMany; i++)
    {
         myData[i] = Wire.read();
    }
    flag = true; 
}
16

Hi Koepel,

I'm having difficulty getting the AVR assembly for the C code.

What are the build_flags you used, or the switches for avr-objdump?

GBEM

17

Quite so.

18

One thing I did find very odd about the proposed code under inspection, is that it will behave erratically at first, if the slave is brought up while the master is already attempting to communicate.

If anyone can hint at what might be going on there, It would be very welcome.

GBEM

19

That's why the standard practice is to execute the Roll Call instruction at the Master side for couple of times until the rpesence of the Slave is detected, and it is done in the setup() function.

21

Never mind, I got it.

For AVR Arduino on Platformio, you add these to platformio.ini:

build_flags =
    -save-temps=obj
    -fverbose-asm

Build_type = debug

Then get the c source interleaved disassembly with:

avr-objdump -S firmware.elf > disassembly. txt

GBEM

22

...and maybe not. Whilst it is 100% certain that the offered atomic register operations aren't there, it's a simple trick to place them so.

Keep a watch on the code's distribution page.

GBEM