The Arduino reference mentions reading only, not writing back.
Are these reads and writes executed as atomic operations?
I've been told that a volatile attribute does not enforce atomic access to such variables. So what's actual behaviour?
Using the memory location instead of keeping it in registers.
A interrupt routine begins and ends, so a variable is always read and written. The interrupt routine is very short, so what happens during the interrupt routine is no problem for the data in the memory location.
(assuming that no interrupt can run during the interrupt routine that uses the same variable)
The Arduino loop() can have for-loops and while-loops, and the compiler might keep the variable temporarily in registers to optimize those loops. That means waiting in a while-loop for a variable that is changed in a interrupt requires the 'volatile' keyword.
If a variable is longer than one byte, then the 'volatile' only does the extra reading and writing, nothing else special, not atomic.
Since the Arduino loop() runs over and over again, the code will often work fine without the 'volatile' keyword. However, there are occasions that it is needed. I assume that the compiler is smarter than me, so I use 'volatile' when appropriate.
My apologies to @bperrybap I think that he already wrote what I mention here
Access to shared variables/data between multiple threads is a complex topic.
IMO, the Arduino manuals are not the best source of technical information.
But mainly, as always in life, keep in mind that the absence of a positive doesn't prove a negative.
i.e. just because it was stated as doing something, doesn't mean that it only does that thing or doesn't do anything else unless it specifically states this.
Atomicity is a totally separate concept/behavior from volatile.
volatile can be thought of like a cache directive telling the compiler how often it must refresh from/to physical memory. Set it and it forces the compiler to read/write it every time it is referenced vs saving/holding it in a register for later use.
Also, accessing the contents of a variable/memory location atomically is different than being able to update/modify it atomically.
All these this stuff really rears its ugly head on a 8 bit processor like the AVR as some of these issues do not exist on other processors.
--- bill
The Arduino reference mentions reading only, not writing back.
this is a poor C++ reference… don’t use it as a complete source of truth, they take many shortcuts there.
A bit rough to read but this is more like it cv (const and volatile) type qualifiers - cppreference.com
- volatile object - an object whose type is volatile-qualified, or a subobject of a volatile object, or a mutable subobject of a const-volatile object. Every access (read or write operation, member function call, etc.) made through a glvalue expression of volatile-qualified type is treated as a visible side-effect for the purposes of optimization (that is, within a single thread of execution, volatile accesses cannot be optimized out or reordered with another visible side effect that is sequenced-before or sequenced-after the volatile access. This makes volatile objects suitable for communication with a signal handler, but not with another thread of execution, see std::memory_order). Any attempt to refer to a volatile object through a glvalue of non-volatile type (e.g. through a reference or pointer to non-volatile type) results in undefined behavior.
I dare to disagree. Volatile can require additional references to the variable, because it can be referenced or updated by parallel code, and then it's crucial that all references are atomic.
If atomicity can be guaranteed only for byte size variables, then it should be noted that volatile can not be applied to multi-byte variables on 8 bit controllers.
A pattern around that restriction are local variables that hold the volatile multi-byte values under control of application code. At the same time the non-local variables should not be attributed volatile, to prevent non-atomic references inserted by the compiler.
The only alternative is a compiler that automatically generates all references to volatile multi-byte variables as atomic operations.
well, volatile in itself does not imply atomic R/W access as per the C++ standard. The compiler generated code does not guarantee that the access will be atomic, just that the lifetime of a register copy has to be limited and that limited optimisation can be done around it.
it can (and often is) by the C++ standard. If you want atomic access to your volatile variable (very very often required), you need to code for it. critical sections like blocking interrupts, using semaphores, ...
➜ so it's two separate concepts. I think that's what @bperrybap was trying to say
Have a look at the HardwareSerial.cpp code for example and you'll see that they support multibyte for the Tx or Rx buffer index
#if (SERIAL_TX_BUFFER_SIZE>256)
typedef uint16_t tx_buffer_index_t;
#else
typedef uint8_t tx_buffer_index_t;
#endif
#if (SERIAL_RX_BUFFER_SIZE>256)
typedef uint16_t rx_buffer_index_t;
#else
typedef uint8_t rx_buffer_index_t;
#endif
this type is used for the instance variables, that are defined as volatile:
volatile rx_buffer_index_t _rx_buffer_head;
volatile rx_buffer_index_t _rx_buffer_tail;
volatile tx_buffer_index_t _tx_buffer_head;
volatile tx_buffer_index_t _tx_buffer_tai
and thus in the code they allow for atomicity
you'll see things like this
int HardwareSerial::availableForWrite(void)
{
tx_buffer_index_t head;
tx_buffer_index_t tail;
TX_BUFFER_ATOMIC {
head = _tx_buffer_head;
tail = _tx_buffer_tail;
}
if (head >= tail) return SERIAL_TX_BUFFER_SIZE - 1 - head + tail;
return tail - head - 1;
}
where the local copies are made in a protected block
That's why referencing the standard here is not really helpful. Should it not be noted that use of volatile variables results in undefined behaviour, except if atomic operations are guaranteed on a machine with the variable?
That's the pattern I already suggested. But what's the influence of volatile to that code? What would change if the indices were not attributed volatile?
sure this is a fair point (I would not call that UB as this is a compiler concept but rather possible erratic behavior) and exactly what @bperrybap was trying to say when he wrote "Atomicity is a totally separate concept/behavior from volatile"
so I did not understand why you "dared to disagree" with that statement. Often You'll need both concepts to get your code right.
And often both concepts have to be considered together, with all bells and whistles, in order to get the code right. Volatile variables required atomic operations in order to make code execute right.
IMO this is just the case with atomic operations and volatile variables.
that's usually the case indeed - but still separate concepts isn't it? (which was my point)
I bring this up because I believe this relates to the OPs questions about atomicity and queuing related issues.
Not really. Go look closely at the actual code.
While they have a conditional to change the indexes to multi-byte ints, the code will have issues should it ever be done.
i.e. while the indexes will bump to ints when necessary to support larger buffers, the actual code was not written properly to support it.
They even have a comment about it in the header file just above the conditionals that set the type.
Some of what is in that github issue mentioned is that the code didn't always properly mutex critical sections of code to ensure atomicity because due to the way to code/system worked, it didn't have to.
While it is sometimes not necessary and you can get away without doing it due to how the overall system operates, the code can cease to work properly once something changes. And that is what happened when they changed how serial output worked from ISRs.
There are more/other issues than what is discussed in that github issue.
Like available() can return an incorrect value when ints are used.
IMO, the current code should error off and fail to compile if the buffer size requires more than an 8 bit index since the code breaks with larger than a 8 bit index.
Years ago like 10+, the code was even worse.
It always used ints with no protections of any kind.
They used volatile but that that doesn't solve the atomicity issues.
note: it only "worked" back then because they never used buffers large enough to need the other byte so it effectively worked as if it was an 8 bit index
I pointed out the atomicity issues to them and suggested that they change the code to use 8 bit indexes which made the code smaller and faster and suggested it was reasonable since the code wouldn't work with ints anyway>
They didn't want to do it and I'm convinced that they didn't even understand the issues.
It was somewhat similar to the experience I had the first week I used Arduino and found atomicity issue in the digitalWrite() code.
Back then the code didn't ensure that the port update was atomic.
I explained the issue in detail and I even offered the solution, but it took 9 months get the fixed code into the release.
To this day, I still believe Mellis never understood the issue.
He argued that there was no way a foreground task could corrupt a register even if interrupted by an ISR. But that is exactly what happens.
He only relented on adding the fix since the servo library only worked properly with the fix in place.
IMO, the early Arduino.cc development team was very green and simply didn't understand many low level h/w and programming issues.
My suggestion years ago when they were having some Serial buffering issues was to re-write the code to use pointers and a counter rather than indexes.
This would make the code smaller and much faster on the AVR than using indexes.
But I got no support and they instead decided to try to add support for larger buffers and indexes and ended up with the current code that doesn't even work with the larger buffers.
The net result was code that was a bit larger and slower than it could be and still doesn't support larger than 256 byte buffers.
Like I said, sharing data between threads is not easy and often even experienced s/w people can get tripped up and screw it up.
IMO, the HardwareSerial code is great example of this.
--- bill
ah - I never looked close enough ! shame on them (and on me for not looking and using that as an example)
thx
Huh, the discussion has drifted into some details, which are far beyond my skills 
But I like to thank you all for your contribution. I guess I gained understanding in what is atomic and what not ... thanks @bperrybap, but also @DrDiettrich, @Koepel and @J-M-L 
So I ask you understanding that I simply can not reply to all your responses .... at least not know, because:
Unfortunately, to actually check proper working of my buffer, I just turn out to be to stupid to set-up proper supporting function (well, not exactly, I'm trying to setup a new class (then to be used for buffer test, but also other porposes), but right now I failing in creating this class to work as intended 
)
So give me some time, I'll keep you posted.
Until then, for your entertainment, a picture of my HW-setup, which I used to test all the stuff. Not completed yet, but still quite good for HW-in-the-loop-tests.
Hu, no. It would be like blocking a keyboard by the connected PC because the PC hasn't yet processed the latest key ... uhm, no.
Actually that does happen.
Even when using a PC keyboard, you will start to get beeps from the machine when the keyboard buffer fills and no more input is accepted.
This was more common back in the old DOS days since the DOS and the BIOS were so crude. i.e. if the app crashed or got really slow or hung up, this would happen.
For your queuing in this project, the simplest thing to do is to set your shared variables to volatile and then mutex their access and updates in the foreground code.
(i.e. block interrupts when messing with them)
All this other stuff in the discussion, is when the code is trying to get fancy for some reason (or being careless) and not using the standard and reliable mechanisms to protect the threads from each other when accessing shared data/variables.
Once that works, if there is some underlying performance need, you can look at some more sophisticated ways to handle the shared data.
But in general, the other ways that avoid using normal locking and volatile mechanisms are not going to be a miracle bump in performance. It typically will only offer slight improvements which is why it is often not worth going after.
To get a big bump in performance, you will need do something differently in the system. i.e. change bus speed, run a different communication protocol, change when/how messages are processed, remove/change other code in the system that is adding timing overhead such as outputing information to display less often, etc...
big/other stuff like that is where you will get the biggest performance/througput gains.
--- bill
That is an old story, isn't it? I do remember this beeping, but today I see my an application where I need to enter my password the letters poping up quite delayed when the PC is still in booting/connecting to the netword-process. Is the data stored in the keyboard or in the PC?
Anyway, buffering is needed somewhere.... and, being honest, putting it into the Master will have tremendous effect on (already written) software. So probably not best SW architecture, but, as highlighted, I just want to take care the seldom situations when I(!) get mad and fool around with all the pots and knobs of my Master-Unit ... 
Coming back to question wether 8-bit assignments are ato ... well, ok, only single instructions are atomic, and if a write to RAM requires two instructions to do so, the C++ - code may look atomic, but it probably isn't.
Even though I still struggle with this view. Provided there are no intermediate results written to RAM, then it would be a quite stupid HW design if a memory access on the ATmega328P would require actually TWO instructions... And, ... ahm, no, I simply don't know enough about AVR architecture.
I have cleaned-up my program, and removed all stuff not dealing with I2C bus. The remaining code is still compiled without errors, and does the Serial.Print.
You will see no volatile, no enable/disable of interrupts.
With respect to volatile:
Is it needed? Buffer itself is somehow "protected" by indicies readIdx and writeIdx - they are just a byte long and accessed once(!). So why volatile then.
With respect to enable/disable interrupts:
I have simply no idea where an interrupt would disturb the execution of loop() significantly. Latest data may not be taken, ok, but next loop of loop() it will.
So, code is working properly. My question is just: Is there a hidden trap because reading/writing of indicies may cause strange side effects (which I haven't seen yet) ... so I'm curious, I try to better understand.
Your comments are welcome. And here is the code:
#include <Arduino.h>
#include <Wire.h>
// Some enumeration to make life easier
enum class OpMode : byte {Sleep, WakeUp, Awake, LightOn, FallASleep, ClearBuffer};
enum Mode : byte {Normal, Configure};
enum Cmd : byte {PowerOff, PowerOn, LightOn};
// I2C BUS
#define I2C_BUFFER_DEPTH 6 // There will be always 6 bytes send
#define I2C_BUFFER_SIZE 16 // Buffer can up to (SIZE-1) messages, shall be 2^n
#define I2C_BUFFER_MASK (I2C_BUFFER_SIZE - 1) // Used in buffer code for fast implementation
#define POWER_ADDRESS 0b0001000 // I2C-Adress of PowerStage
// Define type for I2CBuffer
struct I2CBuffer {
byte data[I2C_BUFFER_SIZE][I2C_BUFFER_DEPTH];
byte readIdx; // Index into oldest row with data
byte writeIdx; // Index into empty / writeable row of data
boolean Overflow; // Will be true if overflow has occured
boolean receiveOk; // Will be false if received data is not ok
};
// Declare I2CBuffer
I2CBuffer i2cBuffer = {{},0,0,false,true}; // No Overflow, received is fine
// ================================================================================================
void receiveEvent(int byteNum) { // receiveEvent just takes care for receiving data
byte cnt = 0; // no byte received yet
byte message[I2C_BUFFER_DEPTH]; // Intermediate buffer for I2C-message
byte next; // next value buffer.writeIdx, calculate in advance
next = ((i2cBuffer.writeIdx + 1) & I2C_BUFFER_MASK); // cycling [ 0 ... I2C_BUFFER_SIZE-1 ]
while (Wire.available()) {
byte in = Wire.read(); // always read, until buffer is empty, but take ...
if (cnt < I2C_BUFFER_DEPTH) { // ... only the number expected fitting to buffer
message[cnt] = in;
}
cnt++;
}
if (byteNum == I2C_BUFFER_DEPTH) { // Number of received bytes as expected?
if (i2cBuffer.readIdx == next) { // Y: Overflow of buffer? (one row remains unused)
i2cBuffer.Overflow = true; // Y: just indicate, do nothing more here
}
else { // N: Data seems ok, so fill Buffer with new data
for (byte col = 0; col < I2C_BUFFER_DEPTH; col++) {
i2cBuffer.data[i2cBuffer.writeIdx][col] = message[col];
}
i2cBuffer.writeIdx = next; // Update writeIdx ro next empty row, atomic!
}
}
else {
i2cBuffer.receiveOk = false; // N: Message not as expected, no copy but report
}
}
// ================================================================================================
void setup() {
// put your setup code here, to run once:
Serial.begin(115200);
// I2C-Bus
Wire.begin(POWER_ADDRESS);
Wire.onReceive(receiveEvent);
}
// ================================================================================================
void loop() {
// Data flow control for state machine
static OpMode opMode = OpMode::Sleep; // Sleep after big-bang
static OpMode opModeLast = OpMode::Sleep; // dito
// Power LED commands
static Mode cmdMode = Normal; // Commanded Mode of CtrlUnit (=Normal,Configure,)
static Cmd cmdParameter = PowerOff; // and associated commands
static int cmdFilterTau = 0; // Normal: time constant of 1st order filter
static int cmdBrightFac = 0; // normalized brightness
static int cmdColorT = 0; // reference colortemperature
// I2C Buffer
byte i2cWriteIdx = i2cBuffer.writeIdx; // ISR-changed, but reading just a byte always fine
byte i2cReadIdx = i2cBuffer.readIdx; // locally here updated
byte i2cReadIdxNext = ((i2cReadIdx + 1) & I2C_BUFFER_MASK); // Calculate in advance
boolean i2cOverflow = i2cBuffer.Overflow; // True, if overflow has occured
boolean i2cReceiveOk = i2cBuffer.receiveOk; // Ture, if Number of reveived byte is as expected
// ================================================================================================
// R E A D I N P U T D A T A
// ================================================================================================
if ((!i2cOverflow) && i2cReceiveOk) { // No errors on bus in receiving data?
if (i2cWriteIdx != i2cReadIdx) { // New message received (indices are different then)
switch (i2cBuffer.data[i2cReadIdx][0] >> 4) { // What mode the Powerstage shall work with?
case 0: // Normal Mode
cmdMode = Normal; // Y: read ALL remaining data
switch (i2cBuffer.data[i2cReadIdx][0] & 0x0F) { // Translate cmd into local meaning (enum)
case 0: cmdParameter = PowerOff; break;
case 8: cmdParameter = PowerOn; break;
case 12: cmdParameter = LightOn; break;
default: cmdParameter = PowerOff; break;
}
cmdFilterTau = i2cBuffer.data[i2cReadIdx][1];
cmdColorT = ((int)i2cBuffer.data[i2cReadIdx][2] << 8) | // ColorT spread on two bytes
((int)i2cBuffer.data[i2cReadIdx][3]);
cmdBrightFac = ((int)i2cBuffer.data[i2cReadIdx][4] << 8) | // BrightFac spread on two bytes
((int)i2cBuffer.data[i2cReadIdx][5]);
break;
case 1: // Configuration Mode. To come ...
default:
break;
}
// Now update readIdx for I2C-Buffer for all Modes of Powerstage
i2cBuffer.readIdx = i2cReadIdxNext; // Writing back to buffer. Should be fine for a byte
// DEBUGGING ONLY: Show powerCMD to evaluate at beginning of loop()
if (cmdMode == Normal) {
byte freeMem = (i2cReadIdx > i2cWriteIdx) ? (i2cReadIdx - i2cWriteIdx) :
(I2C_BUFFER_SIZE + i2cReadIdx - i2cWriteIdx - 1);
Serial.print(F("Mode\t\t: ")); Serial.println(byte(cmdMode));
Serial.print(F("Parameter\t: ")); Serial.println(cmdParameter);
Serial.print(F("Delay\t\t: ")); Serial.println(cmdFilterTau);
Serial.print(F("colorT\t\t: ")); Serial.println(cmdColorT);
Serial.print(F("brightFac\t: ")); Serial.println(cmdBrightFac);
Serial.print(F("FreeMemory\t: ")); Serial.println(freeMem);
if (opModeLast != opMode) {
Serial.print(F("Last Operating Mode\t:"));
if (opModeLast == OpMode::Sleep) Serial.print(F("Sleep"));
else if (opModeLast == OpMode::WakeUp) Serial.print(F("WakeUp"));
else if (opModeLast == OpMode::Awake ) Serial.print(F("Awake"));
else if (opModeLast == OpMode::LightOn) Serial.print(F("LightOn"));
else if (opModeLast == OpMode::FallASleep) Serial.print(F("FallASleep"));
else if (opModeLast == OpMode::ClearBuffer) Serial.print(F("ClearBuffer"));
Serial.print("\t Acutal Mode \t:");
if (opModeLast == OpMode::Sleep) Serial.print(F("Sleep"));
else if (opMode == OpMode::WakeUp) Serial.print(F("WakeUp"));
else if (opMode == OpMode::Awake ) Serial.print(F("Awake"));
else if (opMode == OpMode::LightOn) Serial.print(F("LightOn"));
else if (opMode == OpMode::FallASleep) Serial.print(F("FallASleep"));
else if (opMode == OpMode::ClearBuffer) Serial.print(F("ClearBuffer"));
}
Serial.println(" ");
}
}
}
else {
cmdMode = Normal; // Shut down in normal Modus, create feasible
cmdParameter = PowerOff; // ... command to shut down
cmdFilterTau = 100; // ... if needed, or directly. State machine below
cmdColorT = 0; // ... will take care of it
cmdBrightFac = 0;
}
// ================================================================================================
// O P E R A T E L I G H T C O N T R O L
// ================================================================================================
switch (opMode) {
case OpMode::Sleep:
if (cmdMode == Normal) {
}
break;
case OpMode::WakeUp:
if (cmdMode == Normal) {
}
break;
case OpMode::FallASleep:
if (cmdMode == Normal) {
}
break;
case OpMode::Awake:
if (cmdMode == Normal) {
}
break;
case OpMode::LightOn:
if (cmdMode == Normal) {
}
break;
case OpMode::ClearBuffer:
opModeLast = opMode;
opMode = OpMode::Sleep;
break;
default:
break;
}
}When the Arduino Slave receives data, the interrupt of the Wire library stores the data in a buffer and when that is finishes the receiveEvent() function is called. Then that buffer gets emptied with Wire.read() and you store it in a message[] buffer. Then you copy that into the i2cBuffer.
There are three buffers involved, and maybe another one in the Wire library.
Can you do make it simpler:
void receiveEvent(int byteNum)
{
if (byteNum == I2C_BUFFER_DEPTH)
{
// set some variables, and so on
Wire.readBytes( i2cBuffer.data[i2cBuffer.writeIdx], byteNum);
// increase a few other variables, and so on
}
}
The buffer gives you time to deal with data, but if you don't have that time, then it is not fail safe. Without 'volatile' and without disabling the interrupts to copy variables, you will not know for sure what went wrong and how many data is wrong.
Examples where you don't have the time to process data are:
- If someone adds code that writes a lot of data to a SD memory card.
- If the Master pounds the Slave with I2C sessions, then the loop() will barely run (with an Arduino Uno).
I still think that a buffer is trivial. A good sketch in the Master should not continuously pound the I2C Slave with I2C messages.
Oh, yes, thanks for the observation. Code modified.
Unfortunately that functions seems not to be available on Arduino Uno. So I had to stick to the old while-loop.
And I added quite a lot of volatiles and ATOMIC things, because ...
... there aren't any serious timing requirements.
Well, this is to avoid. And that's why I struggle using volatile when I don't have a exact scenario in mind where I alway need latest value ... which is not the case here. And I don't(!) want to share intermediate results.
Besides, as i2cBuffer is global, I guess it is sufficient to declare i2cBuffer as volatile (see code below), but not the local copies in loop()? (even though right now there are volatile).
Well, this is not done. Nobody waits. Latest values are then taken in next loop of loop() - ok, which is somehow waiting, but not in the sense that execution of loop() is blocked.
But isn't it similar to a global variable that one function changes while the other reads it? I.e. would be the consequence that any global variable has to be volatile then?
I start thinking about that ...
Here is the code
#include <Arduino.h>
#include <util/atomic.h>
#include <Wire.h>
// Some enumeration to make life easier
enum class OpMode : byte {Sleep, WakeUp, Awake, LightOn, FallASleep, ClearBuffer};
enum Mode : byte {Normal, Configure};
enum Cmd : byte {PowerOff, PowerOn, LightOn};
// I2C BUS
#define I2C_BUFFER_DEPTH 6 // There will be always 6 bytes send
#define I2C_BUFFER_SIZE 16 // Buffer can up to (SIZE-1) messages, shall be 2^n
#define I2C_BUFFER_MASK (I2C_BUFFER_SIZE - 1) // Used in buffer code for fast implementation
#define POWER_ADDRESS 0b0001000 // I2C-Adress of PowerStage
// Define type for I2CBuffer
struct I2CBuffer {
byte data[I2C_BUFFER_SIZE][I2C_BUFFER_DEPTH];
byte readIdx; // Index into oldest row with data
byte writeIdx; // Index into empty / writeable row of data
boolean Overflow; // Will be true if overflow has occured
boolean receiveOk; // Will be false if received data is not ok
};
// Declare I2CBuffer
volatile I2CBuffer i2cBuffer = {{},0,0,false,true}; // No Overflow, received is fine
// ================================================================================================
void receiveEvent(int byteNum) {
if (byteNum == I2C_BUFFER_DEPTH) {
byte next = ((i2cBuffer.writeIdx + 1) & I2C_BUFFER_MASK); // cycling [ 0 : I2C_BUFFER_SIZE-1 ]
if (i2cBuffer.readIdx == next) { // Y: Overflow of buffer? (one row remains unused)
i2cBuffer.Overflow = true; // --> indicate it
}
else { // N: Data seems ok, so fill Buffer with new data
byte col = 0;
while (Wire.available()) {
i2cBuffer.data[i2cBuffer.writeIdx][col] = Wire.read();
col++;
}
i2cBuffer.writeIdx = next; // Update writeIdx ro next empty row, atomic!
i2cBuffer.Overflow = false; // No overflow
}
i2cBuffer.receiveOk = true; // N: Message not as expected, no copy but report
}
else {
i2cBuffer.receiveOk = false; // Message not as expected, no copy but report
}
}
// ================================================================================================
void setup() {
// put your setup code here, to run once:
Serial.begin(115200);
// I2C-Bus
Wire.begin(POWER_ADDRESS);
Wire.onReceive(receiveEvent);
}
// ================================================================================================
void loop() {
// Data flow control for state machine
static OpMode opMode = OpMode::Sleep; // Sleep after big-bang
static OpMode opModeLast = OpMode::Sleep; // dito
// Power LED commands
static Mode cmdMode = Normal; // Commanded Mode of CtrlUnit (=Normal,Configure,)
static Cmd cmdParameter = PowerOff; // and associated commands
static int cmdFilterTau = 0; // Normal: time constant of 1st order filter
static int cmdBrightFac = 0; // normalized brightness
static int cmdColorT = 0; // reference colortemperature
// I2C Buffer
volatile byte i2cWriteIdx = i2cBuffer.writeIdx; // ISR-changed, but reading just a byte always fine
volatile byte i2cReadIdx = i2cBuffer.readIdx; // locally here updated
volatile boolean i2cOverflow = i2cBuffer.Overflow; // True, if overflow has occured
volatile boolean i2cReceiveOk = i2cBuffer.receiveOk; // Ture, if #bytes received are as expected
byte i2cReadIdxNext = ((i2cReadIdx + 1) & I2C_BUFFER_MASK); // Calculate in advance
// ================================================================================================
// R E A D I N P U T D A T A
// ================================================================================================
if ((!i2cOverflow) && i2cReceiveOk) { // No errors on bus in receiving data?
if (i2cWriteIdx != i2cReadIdx) { // New message received (indices are different then)
switch (i2cBuffer.data[i2cReadIdx][0] >> 4) { // What mode the Powerstage shall work with?
case 0: // Normal Mode
cmdMode = Normal; // Y: read ALL remaining data
switch (i2cBuffer.data[i2cReadIdx][0] & 0x0F) { // Translate cmd into local meaning (enum)
case 0: cmdParameter = PowerOff; break;
case 8: cmdParameter = PowerOn; break;
case 12: cmdParameter = LightOn; break;
default: cmdParameter = PowerOff; break;
}
cmdFilterTau = i2cBuffer.data[i2cReadIdx][1];
cmdColorT = ((int)i2cBuffer.data[i2cReadIdx][2] << 8) | // ColorT spread on two bytes
((int)i2cBuffer.data[i2cReadIdx][3]);
cmdBrightFac = ((int)i2cBuffer.data[i2cReadIdx][4] << 8) | // BrightFac spread on two bytes
((int)i2cBuffer.data[i2cReadIdx][5]);
break;
case 1: // Configuration Mode. To come ...
default:
break;
}
// Now update readIdx for I2C-Buffer for all Modes of Powerstage
ATOMIC_BLOCK (ATOMIC_RESTORESTATE) {
i2cBuffer.readIdx = i2cReadIdxNext; // Writing back to buffer. Should be fine for a byte
}
// DEBUGGING ONLY: Show powerCMD to evaluate at beginning of loop()
if (cmdMode == Normal) {
byte freeMem = (i2cReadIdx > i2cWriteIdx) ? (i2cReadIdx - i2cWriteIdx) :
(I2C_BUFFER_SIZE + i2cReadIdx - i2cWriteIdx - 1);
Serial.print(F("Mode\t\t: ")); Serial.println(byte(cmdMode));
Serial.print(F("Parameter\t: ")); Serial.println(cmdParameter);
Serial.print(F("Delay\t\t: ")); Serial.println(cmdFilterTau);
Serial.print(F("colorT\t\t: ")); Serial.println(cmdColorT);
Serial.print(F("brightFac\t: ")); Serial.println(cmdBrightFac);
Serial.print(F("FreeMemory\t: ")); Serial.println(freeMem);
if (opModeLast != opMode) {
Serial.print(F("Last Operating Mode\t:"));
if (opModeLast == OpMode::Sleep) Serial.print(F("Sleep"));
else if (opModeLast == OpMode::WakeUp) Serial.print(F("WakeUp"));
else if (opModeLast == OpMode::Awake ) Serial.print(F("Awake"));
else if (opModeLast == OpMode::LightOn) Serial.print(F("LightOn"));
else if (opModeLast == OpMode::FallASleep) Serial.print(F("FallASleep"));
else if (opModeLast == OpMode::ClearBuffer) Serial.print(F("ClearBuffer"));
Serial.print("\t Acutal Mode \t:");
if (opModeLast == OpMode::Sleep) Serial.print(F("Sleep"));
else if (opMode == OpMode::WakeUp) Serial.print(F("WakeUp"));
else if (opMode == OpMode::Awake ) Serial.print(F("Awake"));
else if (opMode == OpMode::LightOn) Serial.print(F("LightOn"));
else if (opMode == OpMode::FallASleep) Serial.print(F("FallASleep"));
else if (opMode == OpMode::ClearBuffer) Serial.print(F("ClearBuffer"));
}
Serial.println(" ");
}
}
}
else {
cmdMode = Normal; // Shut down in normal Modus, create feasible
cmdParameter = PowerOff; // ... command to shut down
cmdFilterTau = 100; // ... if needed, or directly. State machine below
cmdColorT = 0; // ... will take care of it
cmdBrightFac = 0;
}
// ================================================================================================
// O P E R A T E L I G H T C O N T R O L
// ================================================================================================
switch (opMode) {
case OpMode::Sleep:
if (cmdMode == Normal) {
}
break;
case OpMode::WakeUp:
if (cmdMode == Normal) {
}
break;
case OpMode::FallASleep:
if (cmdMode == Normal) {
}
break;
case OpMode::Awake:
if (cmdMode == Normal) {
}
break;
case OpMode::LightOn:
if (cmdMode == Normal) {
}
break;
case OpMode::ClearBuffer:
opModeLast = opMode;
opMode = OpMode::Sleep;
break;
default:
break;
}
}You don't have to use: #include <Arduino.h>
The Wire.readBytes() needs a cast to (byte *) for the first parameter.
I am not able to check if the buffer works in all circumstances with the ATOMIC blocks, volatile and static variables.
The normal way is:
volatile bool flag;
volatile struct myData ...
void loop()
{
if (flag)
{
noInterrupts();
make a copy of the data // memcpy to local struct
flag = false;
interrupts();
use the copy from now on
}
}
It is just the data and the flag. That is easier to check in case of a bug.
The interrupt routine can check the flag and increase an error count if the flag was not cleared yet.