This is probably very simple to solve but I normally program in Java, not C++. Any solutions appreciated!
I'm using an Arduino and some WS2812b LEDs (NeoPixels) - a line of 60 LEDs. I read serial data from my computer and display it on the LEDs via the Arduino. I read 3 bytes, 1 byte for each colour channel (R, G and B), for each of the 60 LEDs. That is saved as a byte array which is then sent to the LEDs. The problem is when I write this as an inline method it works correctly, but when I move the first half of the method to a separate function it only correctly updates the first 40 or so LEDs. Despite this, the two methods look identical to me so I'm quite confused why it isn't working!
The byte array below (b) is correct (what my PC sent) when it is acquire as in block 2, but only the first 60-70% of LEDs are correct when b is acquired as in block3 (the last 30% are the wrong colours).
//unsigned char * b = readBytes(LEDsPerStrip * 3); // For code block 2
//unsigned char * b = bytes; // For code block 3
for (int i = 0; i < 60; i++) // For each of the 60 LEDs
{
LEDs.setPixel(i, b[i * 3], b[i * 3 + 1], b[i * 3 + 2]); // Update each RGB channel (60 x 3 bytes = 180 bytes)
}
When the following code is put above the previous code block everything works fine!
unsigned char bytes[180]; // 60 LEDs x 3 RGB channels = 180
int m = 0;
while (m < 180)
{
if (Serial.available())
{
bytes[m++] = Serial.read(); // Read each byte one by one when available
}
}
However, when I move block 2 to a separate function it stops working completely correctly - despite being identical in my mind...
unsigned char * readBytes(int n)
{
unsigned char bytes[n];
int m = 0;
while (m < n)
{
if (Serial.available())
{
bytes[m++] = Serial.read();
}
}
return bytes;
}
Thanks for any help!
You can't return a pointer (well, you shouldn't) to a variable with local, non-static scope.
unsigned char bytes[n];
A local variable that goes out of scope when the function ends.
return bytes;
And, yet, you return a pointer to it, as though it will continue to exist. That is a REALLY bad assumption.
You need a fixed sized array, passed by reference to the function. There are no shortcuts like this crap allowed.
I'm just starting to program in C++ and was surprised to find it's a little different to Java and the approach I would have used in Java (returning a whole array) wasn't an option.
So if I understand you both correctly - if I want to have a separate function then I must pass a pre-made byte array to that function as an argument rather than returning a pointer. I didn't realise that the pointer approach wasn't advisable. I'll change my method now and hopefully it will work.
Thanks for your answers!
The Arduino is a very small program space compared with a PC.
Just use global variables - it saves an awful lot of trouble (and annoys the hell out of the C purists :)).
...R
and annoys the hell out of the C purists
Especially when it is unnecessary.
Robin2:
The Arduino is a very small program space compared with a PC.
Just use global variables - it saves an awful lot of trouble (and annoys the hell out of the C purists :)).
Aww man... no! 
Since the array length (n) is known -- as opposed to being dynamic with a while (Serial.available()) or some such -- there's no reason not to pre-define the array and pass the pointer.
I know uC programming is different and thus there are exceptions to traditional best practices, but using globals just defeats the point of modularizing code into functions. If you still have all these symbol dependencies, why not just make it an anonymous block of code right in the function body instead?
IMH(?)O, functions should be as ignorant as possible to the symbols defined outside of their scope -- micro or not. It makes it so much easier to debug and re-use code when there aren't all these little interdependencies and side-effects going on everywhere.
-- C Purist. ]:)
I'm just starting to program in C++ and was surprised to find it's a little different to Java and the approach I would have used in Java (returning a whole array) wasn't an option.
Why were you "surprised". Did you think they were the same ?
C/C++ is much older than Java and there is a lot more stuff that the programmer has to pay attention to. Like memory allocation.
It must be very difficult to learn Java and then try and get on top of C. Makes me glad to be old and therefore done the other way around.
try
**new code
const char * readBytes(char *inArray, int arraySize)
{
//send in an empty allocated array of your choosing
int m = 0;
while (m < arraySize)
{
if (Serial.available())
{
inArray[m++] = Serial.read();
}
}
return inArray;
}
Java and modern languages have garbage collection. Even Objective C didn't have GC until recently. You had to clean up your own mess. GC does add extra resources to the app. You'll never see it in the Arduino.
... and you still return a dangling pointer.
No matter how you implement the loop, if the array is allocated on the stack, and a pointer to that array is returned, it will point to memory that is no longer allocated to the stack by the time you use it, and thus, can be literally any sequence of bytes that happens to be physically in those memory locations at the time.
why not continue the program instead of return ???
If you leave out return, gcc will probably yell at you for not returning a value in a function that was declared with a return type.
This is a problem solved easily with objects.
To completely remove the function situation without explicitly writing the code inline, we can put the code into an object, now a simple declaration provides us with the information.
struct InputByte{
InputByte() : data( waitRead() ) {}
operator char&() { return data; }
InputByte &operator =( char c ){ return data = c, *this; }
static char waitRead(){
while(!Serial.available());
return Serial.read();
}
char data;
};
It can be used quite easily:
InputByte b[ 30 ];
b[29 ] = 0;
Serial.println( ( char* ) b );
If you can wrap you head around this, a template will provide a different solution, which can also be used just like an array.
inline char waitRead(){
while(!Serial.available());
return Serial.read();
}
template< unsigned N > struct CaptureString {
CaptureString() : data( N ? waitRead() : N ) {}
operator char*(){ return (char*) this; }
char data;
CaptureString< N && ( N < 0xFFFF ) ? N - 1 : 0xFFFF > next;
};
template<> struct CaptureString< 0xFFFF >{};
And its use is like below ( equivalent to the non template version above )
CaptureString< 30 > c;
Serial.println( c );
Edit: fixed code error in template version.
sirn goto
pYro you don't give the need to use template
vector0:
sirn goto
pYro you don't give the need to use template
Goto has no place in logical code.
Sorry, the template parameter takes the number of values required + 1 for the null
goto is perfect : it do only what you want
it's not as you prefer to read Very_Long_Code that make goto's not logic
logic is in mind not in code as the compiler will do what it want
I'm a fan of using the right tool for the job, and I think people get overly pedantic about not using goto, for nothing more than some cargo-cult notion that goto == bad programming. It's a valid statement that was included into the language for a reason, and there are times when it makes no sense to bend your code into contortions to prevent using a syntactic element that boils down to the same instruction the compiler will probably end up using anyway to undo the "not technically goto" code you otherwise have to write.
E.g., break and while(1) are synonymous with goto. So why not use whatever makes the code easiest to understand, and forget all the BS stigma? Good and bad code can be written in any language by any one with any collection of statements. The anti-goto movement was borne out of a desire to avoid spaghetti code and has been blown so far out of proportion that folks will honestly prefer an intricate series of flag variables and state checks just to say they didn't use goto. Does that really make things any cleaner / easier / better / faster?
A program exists to perform a task first, and be as obvious as possible to human beings that have to maintain it, second. Those are the true goals of programming. Anything that helps achieve those goals is fair game, IMO.
BUT...
I don't see where goto fits in this problem. You can't goto out of a function, for example. (Please no one bring up long jumps... DOH! That is truly a tool for a very specific job.)
I just can't not comment. I designed and built software for drug design in c, c++, java, and objective c for 30 years and supervised many, many of programmers. I could count on one hand the number of instances where a goto was actually a reasonable alternative. In every single case, it was a reasonable alternative only because the inherent design was faulty.
If any programmer came to me with code to review with a goto in it, I would immediately red flag that programmer. It isn't that a goto is bad in itself, it is just a symptom of poor design and the inability to recognize that poor design.
OK, then humor me. What's the 30-years' experience alternative to this typical situation?
int hwdev_sendmsg (char *data, size_t data_len, uint32_t timeout) {
uint32_t now = millis();
uint8_t flags;
int rval;
// This is timing critical
flags = SREG;
cli();
// Initialize hardware
rval = dev_init();
if (rval != ERR_OK) goto cleanup;
// Set hardware to whatever appropriate mode
rval = dev_set_mode(SOME_FLAG);
if (rval != ERR_OK) goto cleanup;
while ( ((rval = dev_query()) != ERR_OK) && (data_len != 0) ) {
// Check for timeout
if ( (rval == ERR_BUSY) && ( (millis() - now) < timeout) ) {
goto cleanup;
}
// Check for fatal error
if (rval == ERR_FATAL) {
goto cleanup;
}
// Send data to device
while (data_len) {
rval = dev_send8(data);
// Device will signal when its buffer is full
if (rval == ERR_BUFFER_FULL) {
// Fall back to wait loop
break;
}
// Handle other errors
if (rval != ERR_OK) {
goto cleanup;
}
// Update data pointer and size
data++;
data_len--;
} // inner loop
} // outer loop
// If data has been sent successfully, rval will be ERR_OK.
// Otherwise, the relevant error message will be returned.
cleanup:
// We're done with the device now
dev_close();
// Restore previous state
SREG = flags;
return rval;
}
This is an anonymous and contrived example -- not a direct copy of a real driver, but close enough to a LOT of loops-within-loops I've had to write recently. (E.g., file read loop with a buffer processing loop within.) The cleanup portion is fairly benign here. In real code, it can easily be half a dozen statements long. You could put those inline anywhere there's a goto above, or use a flag variable and bail out if it's set, or any of a dozen other techniques... But I fail to see what's so gosh-darn diabolical about the obvious and tidy goto approach? It works, the purpose is clear, the code path is trivial to follow, it keeps you from having to write redundant cleanup code (and these loops are fairly short, with few exit points -- there could be a lot more!), and it prevents errors if you have to free memory or restore flags or whatever else you might forget to do otherwise.
A LOT of very real, very thoroughly vetted, production-level code uses this pattern. I cannot imagine, if there were truly something harmful about it, that you would see this in so many ubiquitous codebases -- like the Linux kernel, OpenSSL, Firefox ....
Nonetheless, I am willing to hear the counter-argument if someone feels compelled to plead their case. I don't expect it'll make the wholesale ban any less nonsensical to me, but who knows?