Is there a good place to post code for reviews?

Projects General Guidance
1

Hey, I'm pretty new to working with hardware at all, and I have a little robot project I'm working on.

I was wondering if there is somewhere I can post code for review so folks can point out better ways I could have done something. Is there a forum here that is better for that kind of thing?

Specifically, I wrote a sketch to look at a couple of 2-axis joysticks and send them out to an xbee.

The code does what I think it should (though I haven't implemented the code to receive it and parse up the instructions, so I'm hoping that my way of encoding the data won't be a pain to work with). However, my C is a bit iffy (don't laugh, but I mostly do front-end JS and PHP web work for a living), and the code seems super inelegant to me. I was hoping for a way to loop through the analog in pins, and although I understand that hashes aren't a good way of doing that, I couldn't think of a nice loopy way to write what I am doing without doing something like that, which is what I'd do in the other languages I use.

And I'm sure there are other crappy things or more idiomatic ways of working in my code.

So... is there any good place to get code reviews?

2

If is it Arduino related then you can post it here and we will give feedback. Not arduino related, the post is removed.

3

Hi and welcome !

Your code doesn't seem crappy to me, but... my C is quite iffy as well :slight_smile:

Posting code here can be done by clicking the #-button of the message menu. That makes it easier to copy&paste&review code for others.

Messages may.... indeed get removed or placed in another section if they're not on topic or posted in the wrong section, but... I wouldn't worry too much about that. Arduino was developed to introduce people with electronics and most moderators realize that.

4

Is check_Left_Yposition() really checking anything? Function names should clearly define, not vaguely hint at, what the function does.

sanityInt looks pretty useless.

Sending serial data happens one byte at a time, whether you send it one value at time or piss resources away stuffing the data into a String. So, quit wasting resources on String instances. Send the data when you have it.

5

SanityInt looks pretty useless.

Sending serial data happens one byte at a time, whether you send it one value at time or piss resources away stuffing the data into a String. So, quit wasting resources on String instances. Send the data when you have it.

SanityInt was part of a loop that I had been using so that something was getting sent out the serial every time through the loops, and I didn't catch that when I posted my code.

The thing about "send serial data when you have it" is useful, and I am refactoring around that. I wish I had a better way of looping through a hash of pins and names.

6

Here is how I ended up refactoring it. However, it still feels like I should be able use some kind of structure I can iterate through, instead of repeating a bunch of statements and changing the name of the output variable and pin.

/*
  2-stick Serial Controller w/ xBee functionality
  
 */

#include <SoftwareSerial.h>

#define echoToSerialOut true

#define rxPin 2
#define txPin 3
#define ledPin 13

#define left_stick_Y       A1
#define left_stick_X       A0
#define left_select        6

#define right_stick_Y      A3
#define right_stick_X      A2
#define right_select       7

SoftwareSerial xbee(rxPin, txPin); // RX, TX

#define delimiter "/"
#define terminator "//\n\r"

void setup()  
{
  pinMode(ledPin, OUTPUT);    
  pinMode(rxPin, INPUT);
  pinMode(txPin, OUTPUT);

  pinMode(left_select, INPUT);
  pinMode(right_select, INPUT);
  xbee.begin(9600);
}

void loop() 
{
  
  //check_Left_Yposition();
  xbee.print("LEFT_Y:"); 
  xbee.print(map(analogRead(left_stick_Y), 0, 1023, -100, 100)); 
  xbee.print(delimiter);
  
  //check_Left_Xposition();
  xbee.print("LEFT_X:"); 
  xbee.print(map(analogRead(left_stick_X), 0, 1023, -100, 100)); 
  xbee.print(delimiter);
  
  //check_Left_select:  
  xbee.print("LEFT_SEL:"); 
  xbee.print(digitalRead(left_select) == LOW ? "1" : "0"); 
  xbee.print(delimiter);
  
  //  check_Right_Yposition();
  xbee.print("RIGHT_Y:"); 
  xbee.print(map(analogRead(right_stick_Y), 0, 1023, -100, 100)); 
  xbee.print(delimiter);
  
  //  check_Right_Xposition();
  xbee.print("RIGHT_X:"); 
  xbee.print(map(analogRead(right_stick_X), 0, 1023, -100, 100)); 
  xbee.print(delimiter);

  //check_Right_select();
  xbee.print("RIGHT_SEL:"); 
  xbee.print(digitalRead(right_select) == LOW ? "1" : "0"); 
  xbee.print(delimiter);
  
  xbee.print(terminator);
  
  // wait 2 milliseconds before the next loop
  // for the analog-to-digital converter to settle
  // after the last reading:
  delay(2);
}
7

it still feels like I should be able use some kind of structure I can iterate through

There is. It's called an array. Put the pin numbers in an array, and use a for loop. Actually, two arrays z\and two loops, since you have analog and digital pins/functions involved.

8

You program in C pretty well for a PHP guy! :smiley: I keed, I keed....

C doesn't have a native hash type, just an array. PHP's implementation of arrays is kinda weird (but what else would you expect?), as you can either build it using a numeric index:

$var[0] = ....;

... or, using key/value pairs:

$var['a'] = 'b';

... while in C, you have to define its size up-front, and then you can only use numeric indices between 0 and <array length - 1>. Here's an example using an array of structs:

typedef struct {
  char  caption[8];
  int  pin;
  int  min;
  int  max;
} analog_t;

analog_t  stick[4] = {
  { "LEFT_Y", A1, -100, 100 },
  { "LEFT_X", A0, -100, 100 },
  { "RIGHT_Y", A3, -100, 100 },
  { "RIGHT_X", A2, -100, 100 }
};

void setup () {
  ...
}

void loop () {
  uint8_t  i = 0;  // Loop counter
  
  for (i = 0; i < 4; i++) {
    xbee.print(stick[i].caption);
    xbee.print(map(analogRead(stick[i].pin), 0, 1023, stick[i].min, stick[i].max));
    xbee.print(delimiter);
  }
  
  // Something similar should be done for digital values
}

Now, since there's no key/value support, you can't get at an array element by some arbitrary tag -- only it's index. The equivalent would go something like this:

// Find the stick[] element for LEFT_Y:
for (i = 0; i < 4; i++) {
  if (strcmp(stick[i].caption, "LEFT_Y") == 0) break;
}

// The variable i should now be set to the index of the matching entry,
// assuming there was a match.  If there wasn't, it'll be set to the
// last entry.  If there's a chance of that happening, you'll have to
// find a way to signal that condition.

xbee.print("The pin corresponding with LEFT_Y is :");
xbee.print(stick[i].pin);

It might seem wasteful to loop through an array looking for matching values, but you have to realize that's all the higher level languages do anyway. You're just spared the ugliness of the mechanism by which it works. :wink:

9

SirNickity, that is better than I had hoped for... thank's so much for the translation. Some of this stuff is tough I learned in college but forgot or never mastered, but that translation was super helpful. The "array of structs" is really what I was hoping someone would point me to, as I know there was a good way of doing it, I just still don't know the language well. Thanks!

10

Glad it helped! :slight_smile: Beware, there are a few traps in that code for those new to C, particularly where it pertains to string handling.

For example:

char caption[8];

This means your string can be 7 characters long, plus the terminating null (ASCII 0x00). When you define a literal string:

char caption[8] = "RIGHT_Y";

... the assignment is allowed when you initialize the char array, but you can't just go around assigning strings that way anywhere else. For instance, this wouldn't work:

char caption[8];

void setup () {
  caption = "LEFT_X";
}

void loop () {
  xbee.print(caption);
  ....
}

The problem here is that "LEFT_X" is an anonymous array of char, which is stored at a particular memory address. You don't actually copy that string to caption, you just set caption (which is essentially a pointer to char) to the memory address where the string literal currently exists. By the time you've left setup() and entered loop(), that memory is technically no longer allocated -- and in fact, may have been *re-*allocated to something else. In other words, caption is going to point to whatever happens to be at that memory location, and it may no longer be the text you intended.

That is why strcpy() exists -- it assigns the contents of a string (another variable or a literal) to a variable. It stops when it finds a Null. String literals always contain a terminating null. When you define a char array, it's important you define it to <length of text + 1> and make sure the byte following your last text character is 0x00. (Again, strcpy() will take care of this.) Make sure the array is long enough for this whole string, otherwise you'll write over whatever existed in memory after your char array. Could be nothing, could be something.

Similarly, you can't compare char arrays and string literals like this:

char caption[8] = "LEFT_X";

void loop () {
  ...
  if (caption == "LEFT_X") {
    // This doesn't work
  }
  ...
}

... because what you are really asking is "is the address contained by caption equal to the address of this anonymous string literal?" This should always fail. Instead, you need to do this:

if (strcmp(caption, "LEFT_X") == 0) {
  // This works
}

The strcmp() function works the same as strcpy() ... it compares the bytes at the respective memory addresses one at a time, until it encounters a null. If they're the same, it returns 0. (It returns positive and negative numbers to tell you which string is "greater", which you won't usually be interested in, but it's worth knowing.)

Apologies if you already know how to deal with C strings, but it's not obvious, and not intuitive until you know.