Problem with this code, Can anyone solve this? I'll buy you a 6-pack :)

Projects Programming
1

Okay guys, I've slimmed my code down as much as possible while showing the problem I'm having, and have had a bunch of times while coding. Since the same symptom has been popping up, I'm assuming I'm doing something wrong or approaching something inefficiently.

I've put a lot of commenting in the code so help you follow it along. The problem I'm having is in the main loop if you look where it's checking a switch and says "switch Pressed". All I get when I press the switch is "sw" - Apparently the Serial write routine never gets to finish for some reason, but it starts. If I uncomment the updateLine function call, then it works and prints "switch pressed." every time I press the switch. however, the same routine is used at the end of my doHandshakeInt routine, and works correctly (I see the update on the screen)

I am confused how the same routine can work somewhere else in the code but when called from another place, it appears to disrupt the running of the arduino code.

Any help would be immensely appreciated, as it would probably not only help me with this project but help me identify what I am doing wrong consistently in coding C++ because this has happened many other times during my coding. I'll paypal you money for a 6 pack if you can help me solve this one :slight_smile:

This is the first C++ program I've ever written so I am sure there are plenty inefficiencies everywhere, I am all ears to suggestions and how to make things better, I'm looking to learn how to get better at this! And yes I know, some of my variable names are goofy...

Code is in the next post. Seems I've exceeded 9500 characters adding it here.

2

Attached file

slim.txt (12.8 KB)

3

After a couple more tests I ran, it appears my issue is actually inside of the newsend routine... If I uncomment that line from my updateLine routine, it does not stop during the serial write. (I have to also uncomment the line for doNextChcmd because that routine calls the newsend routine)

4

I've already made 2 improvements, changed:

void newsend(int* newData, int optional) {

to

void newsend(int newData[], int optional) {

I was using pointers before I learned you could load in a passed array into one in the functions scope (as seen I did with the updateLine routine)

I also put noInterrupts(); at the beginning of the newsend routine and then interrupts(); at the end. I wouldn't want the device to start reading a packet from the radio just before a packet was about to be sent. While this shouldn't ever happen (radio should never request to send data after the device has signaled it wants to send a packet) this is good insurance in case it ever does happen. I've watched on a logic analyzer if the device does not respond to the REQH LOW transmit request, it takes it back high and tries again.

despite these improvements I'm still having the same problem ( "sw" when I press the button ). Anyone have any ideas?

5

The serial routines use interrupts. If you turn them off they will probably stop working. Try commenting out that part of the code your function and see if it fixes it.

6

If I was a betting man, I'd put my money on you running out of memory. You've got several fairly long arrays in there, including quite a few that are ints.

Since I'm not a betting man, I suggest you stick some code in to see how much memory you actually have available at the end of setup. If, as I suspect, you're running out then PROGMEM may be your saviour.

Incidentally, your code layout sucks. It'd be much easier to see the structure if you indented each nested block of code. You do use indentation, but not consistently.

I strongly prefer putting { and } on separate lines indented by the same amount since this makes it much easier to see how they pair up (and where they don't). Putting the opening { at the end of the preceding line is also a widely accepted alternative, although my personal view is that the justification for that disappeared when people stopped working on 25x80 teletypes. But the combination of { and } on the same lines as other statements, and inconsistent indentation, and significant nested control structures, make it hard for me to be convinced that the control structure is actually right.

7

If memory is a problem, then make all your int arrays uint8_t (basically an unsigned 8 bit number). None of the values are bigger than 255, so you can cut the storage requirements in half.

8

Thanks for the responses so far, I've already applied PROGMEM to my variables that do not need to be modified after they are declared. PeterH I'm with you on the indentation, I am usually pretty good with it but I find the Arduino coding environment strange with how it treats tabs. A tab is not truely a tab it just adds a # of spaces and I find myself getting "off-center" with it and end up going back and fixing my indentations after writing code for awhile. I've still got a lot to learn, I am familiar with a lot of programming languages and it's difficult sometimes to keep track of the differences between them especially when learning a new one. I forget which language it was but I had some issues with placing a { or a ( on the next lien rather than at the end of the line with the command so I have gotten into a habit of placing the {'s on the line where they originate, even though I do prefer, as you said, to block the code. I should really make a mental note of which language that was and only program that way when I'm coding in that language. Maybe I'll store it in my PROGMEM :slight_smile:

Marco that's a good idea, I will look into that next. Just before these suggestions came rolling in, I just got my code to work, somehow, I'm not even sure what I did, and this is usually how it goes when this strange behavior takes place, I think that supports Peter's hunch that I am running out of memory, so my next big goal is to shoot for efficiency of my code as much as possible. Applying the PROGMEM changes and integer classifiers I think is a great start.

Anyone else have any suggestions, I'm open to hear them! Thanks for the help so far everybody.

9

Did you say, 6-pack?

OK. 

//////////////////////////// SUBROUTINES //////////////////////
void doHandshakeInt() {
  
  handshakeCompleted = 1;

  // INITIAL HANDSHAKE
  digitalWrite (DATAC, LOW);
  digitalWrite (REQC, HIGH);
  digitalWrite (DATAC, HIGH);
  delay (13);
  digitalWrite (DATAC, LOW);
  digitalWrite (DATAC, HIGH);

  // RECEIVE COUNTER PACKET FROM HU
  // (Interrupts do not trigger while inside an interrupt so
  // this must be done manually inside this ISR routine)

  // This routine could be made more robust by having the
  // type of packet received checked to make sure it was a
  // counter packet and if not, another read could take place.
  // This is a theoretical scenario, I have not seen it happen
  // yet so for simplicity, I am leaving this as is.  If unexpected
  // hangups at start time happen, this is something to try and a 
  // place to look.
  
  waitfor (REQH, LOW);
  delay (1);
  digitalWrite (REQC, LOW);
  dorecv();
  waitfor (REQH, HIGH);
  delay(1.5);
  digitalWrite (REQC, HIGH);
  delay(4);

  paxSiriusID[6] = lastHuCounter;       // Prepare packet with last counter reported by headunit
  newsend(paxSiriusID, lastHuCounter);  // SEND SIRIUS DEVICE ID PACKET
  delay(4.5);
  digitalWrite (REQC, HIGH);
  waitfor (REQH, HIGH);

  digitalWrite (REQC, LOW);
  newsend (paxSiriusID2, 0);            // SEND PACKET TO ENABLE TITLING OPTIONS
  digitalWrite (REQC, HIGH);  
  waitfor (REQH, HIGH);

  if ( verboseLevel == 2 ) { Serial.println ("Handshake completed."); }
 
  char startup1[] = "L1:Kenwood HackT)";
  char startup2[] = "L2:Booting v1.0)";
  updateLine (101, startup1);
  updateLine (102, startup2);
  return;
}

Major problems here.

  • delay() doesn't work (properly) inside an ISR, so this will potentially hang for ever, at the delay() calls.
  • Serial.println won't work properly inside an ISR, as it uses interrupts. It might work for a while until the buffer fills up.
  • updateLine does too much. ISRs are supposed to be short.
  • delay(1.5); : delay takes an unsigned long. You can't delay 1.5 milliseconds.
  • I'm uneasy about using waitfor. That is also delaying your ISR

Basically ISRs are supposed to be short. Save something to a buffer, or set a flag. Not all that stuff. Not by a long way.

Similar remarks for your other ISR:

void doRead() {
  if ( digitalRead(REQH) == HIGH ) {return; } else {
    digitalWrite (REQC, LOW);   // Accept radios request for transmit by setting REQC low
    digitalWrite (SETSS, LOW);
    int i = 0;
    while (i < 100) {  // Packet length will never be more than 100
    incoming[i] = SPI.transfer(255);  // must send bogus data to get data ( simultaneous transfer )

    // The 5th byte ( [4] ) contains the number of remaining bytes in the
    // packet.  This is used to calculate the size of the packet; break
    // out of this while loop once the last byte has been received.
    if ( incoming[4] && i == incoming[4]+4 ) { break; }
    i++; // increment for loop counter
  }

  digitalWrite (SETSS, HIGH);  // SPI SS goes high after receiving is complete
  lastHuCounter = incoming[6];
  lastRadioRead = millis();
  waitfor (REQH, HIGH);       // Confirm transmittal is done by checking for REQH to go high.
  digitalWrite (REQC, HIGH);  // Acknowledge receipt of data by setting REQC high
  unchecked = millis();
//  lastRadioReqTime = millis();
  }
}  // end doRead routine

100 SPI transfers inside an ISR? Too much, again.

Apparently the Serial write routine never gets to finish for some reason, but it starts.

See above, because of the ISR, the serial buffer has probably filled up.

char argh[] = "L1:ARGH)";
      updateLine (101, argh);

Why not:

     updateLine (101, "L1:ARGH)");
10

I do have it in the back of my head to take inventory of all my global variables and see which ones really don't need to be global... I know I have a lot of global variables and I don't think they all have to be. I'm sure that can add up to unnecessary memory usage too.

11

Thanks for the breakdown Gammon. I haven't seen a since my BBS days from 1994 till about 2000ish :slight_smile:

I will experiment with removing the delay's inside the handshake ISR. I've put them there to try and duplicate the communication as accurately as I saw with a logic analyzer when the Kenwood radio was connected to a "real" sirius device, but I have also noticed there is a large window there, as I have had less and more pauses in between signals during my coding and didn't see a problem. It may work just as well with no delays, but I had them there just to try and duplicate what was a professional product.

I had thought to take out all of my serial print lines to see if it worked better/at all but didn't get that far. I will keep this idea on my radar, especially when it comes to inside of ISR's.

I understand what you mean about setting flags and keeping ISR's short, I think one of my next stops should be to see what I can strip from my ISR's and place in my main code, however the only ISR's I have are for the initial handshake (which only happens once per radio-turnon) and one for reading data. The radio pulls a signal line LOW and expects the device to pull it's line LOW to say it's ready and then the transmission happens... No interrupts should happen during this, and surely not a handshake request. So for this situation I think it is okay to have longer ISR's which is why I programmed it the way I did, but if I can keep some of the stuff outside of the ISR's, I might as well get into the habit now.

I will definitely take your suggestion on the updateLine variable bypass! I did not know you could do that, I will be definitely implementing that. Thanks!

Also, that 100 was just a figure I came up with, the SPI transmissions are never more than 30 bytes. I could have encased it in a "while (1)" to be more concise, I wonder if that would take up less memory too. I just needed a way to loop until a break command. At a second glance, that while i < 100 is pretty goofy, like I said earlier in my post I know I have a lot to learn this is all new to me but I have a good headstart with lots of principles of coding I've learned with the other languages that I know.

Thanks for your suggestions I am going to go apply them now!

12

catatung:
my next big goal is to shoot for efficiency of my code as much as possible.

It's possible to measure the available memory (bearing in mind the catch-22 that it takes memory to print out the answer) and I suggest you do that to see whether/how much of a problem you have rather than just try to reduce the memory use and hope the problem goes away. It's the nature of these systems that you are going to be tight on memory in some situations and you need to know just how close to the edge you are.

Nick Gammon's comments all sound very alarming too.

13

Thanks Peter, I did just read up on that. I found a way to do it from the command line, and came up with this:

C:\Documents and Settings\brown\Desktop\arduino-1.0.1\hardware\tools\avr\bin>avr-size "\Documents and Settings\brown\Local Settings\Temp\build8300788516441730106.tmp\playwiththis.cpp.elf"
text data bss dec hex filename
7896 1408 530 9834 266a \Documents and Settings\brown\Local Settings\Temp\build8300788516441730106.tmp\playwiththis.cpp.elf

From this report it appears it uses 1408+530 = 1938. I'm not sure how accurate this method is but that puts my figure extremely close to the limit of 2048, and I have read problems can occur even while you're inside the limit (I suppose during execution some space is needed)

After making the PROGMEM changes and I forget if there was anything else since I initially checked, my figures now are 706 and 530 for a total of 1236, a savings of 702 bytes!

I did a little reading also and see that the Arduino Mega boards have 8kb of SRAM, which is 4x the space I'm working with now. I would rather learn how to be more efficient though, and try to squeeze in what I need with efficiency, not by a lazy memory upgrade. I don't want to be like Microsoft's applications, memory hungry programs written by lazy programmers :slight_smile:

14

Very strange... I applied PROGMEM to a handful of variables that I know don't need to be written to during the course of the code and it broke my code! Now I get "swit" when I press the button, but the screen does update, but I know communication is stopped at that point because there are no more "."'s. I took all the PROGMEM's out and now it works again.

15

Whoops, I was just doing some reading, looks like you have to strcpy the memory location of the PROGMEM variable using a pointer or something to that effect before you can use it.... This makes it seem a little tricky since my variables are different lengths...

16

I'm having trouble with PROGMEM and int arrays; I have about 10 arrays as follows:

const uint8_t keyshort5[] =   {  0,0,21,0,8,80,4,65,76,4,0,1,0  };

Later in my code I have a routine that matches the contents of an INT array "incoming" to see if they match.

If I prefix the above array with PROGMEM, how can I retrieve it to a temporary int array? for example, maybe a declaration of "uint8_t keytest[12];" - How could I load the keyshort5 array in PROGMEM into this array so I can compare the two and see if they match?

It would be even better if I could compare the incoming[] array with the progmem saved array directly but I'm assuming I need a temporary array to load to before I can compare the two...

17

catatung:
Thanks for the breakdown Gammon.

You can call me Nick .

Someone on another thread called me Sir Nick but I haven't been knighted ... yet.

catatung:
If I prefix the above array with PROGMEM, how can I retrieve it to a temporary int array?

You don't need to make a copy.

See here for one approach (there are others):

Look up the progmem-related functions. You should be able to compare/copy/whatever PROGMEM data pretty easily.

18

catatung:
Thanks Peter, I did just read up on that. I found a way to do it from the command line, and came up with this:

No, that is telling you how much space the various segments in your program are taking up. It does not tell you how much free memory you have at runtime. To do that you need to find the size of the gap between the stack and the heap. There is a handy utility function that does this for you. It's not provided by the Arduino runtime, but it's been posted in the playground fairly often and I think a search for memory check or similar would find it pretty quickly.

19

I posted it in this thread (among others): http://arduino.cc/forum/index.php/topic,82772.0.html

20

Thanks for the suggests. Here is specifically what I am trying to do. I have some arrays defined as such:

const uint8_t keyshort5[] =   { 0,0,21,0,8,80,4,65,76,4,0,1,0  };
const uint8_t keyshort6[] =   { 0,0,21,0,8,80,4,65,65,4,9,2,0  };
const uint8_t keyshort7[] =   { 0,0,21,0,8,80,4,65,65,4,0,3,0  };
const uint8_t keyshort8[] =   { 0,0,21,0,8,80,4,65,194,4,1,4,0 };
const uint8_t keylong5[] =    { 0,0,21,0,8,80,3,65,76,4,0,1,0  };
const uint8_t keylong6[] =    { 0,0,21,0,8,80,3,65,65,4,9,2,0  };
const uint8_t keylong7[] =    { 0,0,21,0,8,80,3,65,65,4,0,3,0  };
const uint8_t keylong8[] =    { 0,0,21,0,8,80,3,65,194,4,1,4,0 };
const uint8_t keynext[] =     { 0,0,21,0,8,80,0,65,65,1,0,0,0    };
const uint8_t keyshortam[] =  { 0,0,21,0,8,80,4,192,131,0,94,0,0 };
const uint8_t keylongam[] =   { 0,0,21,0,8,80,3,192,131,0,94,0,0 };
const uint8_t keyprev[] =     { 0,0,21,0,8,80,0,65,65,2,0,0,0    };
const uint8_t keyholdnext[] = { 0,0,21,0,8,80,5,65,65,1,0,0,0    };
const uint8_t keyholdprev[] = { 0,0,21,0,8,80,5,65,65,2,0,0,0    };
const uint8_t keyfm[] =       { 0,0,21,0,8,80,2,65,192,3,0,0,0 };
const uint8_t keyauto[] =     { 0,0,21,0,8,80,2,65,65,6,0,0,0  };

Then later in the program, a received packet has to be checked to see if it matches any of those:

int checkWhichButton() {
    if ( comparearrays (incoming, keyshort5,   12) == 1 ) { lastRadioRead = 0; return   5; } else {
    if ( comparearrays (incoming, keyshort6,   12) == 1 ) { lastRadioRead = 0; return   6; } else {
    if ( comparearrays (incoming, keyshort7,   12) == 1 ) { lastRadioRead = 0; return   7; } else {
    if ( comparearrays (incoming, keyshort8,   12) == 1 ) { lastRadioRead = 0; return   8; } else {
    if ( comparearrays (incoming, keylong5,    12) == 1 ) { lastRadioRead = 0; return  15; } else {
    if ( comparearrays (incoming, keylong6,    12) == 1 ) { lastRadioRead = 0; return  16; } else {
    if ( comparearrays (incoming, keylong7,    12) == 1 ) { lastRadioRead = 0; return  17; } else {
    if ( comparearrays (incoming, keylong8,    12) == 1 ) { lastRadioRead = 0; return  18; } else {
    if ( comparearrays (incoming, keynext,     12) == 1 ) { lastRadioRead = 0; return  40; } else {
    if ( comparearrays (incoming, keyprev,     12) == 1 ) { lastRadioRead = 0; return  30; } else {
    if ( comparearrays (incoming, keyshortam,  12) == 1 ) { lastRadioRead = 0; return  10; } else {
    if ( comparearrays (incoming, keyholdnext, 12) == 1 ) { lastRadioRead = 0; return  41; } else {
    if ( comparearrays (incoming, keyholdprev, 12) == 1 ) { lastRadioRead = 0; return  31; } else {
    if ( comparearrays (incoming, keylongam,   12) == 1 ) { lastRadioRead = 0; return 110; } else {
    if ( comparearrays (incoming, keyfm,       12) == 1 ) { lastRadioRead = 0; return  20; } else {
    if ( comparearrays (incoming, keyauto,     12) == 1 ) { lastRadioRead = 0; return   9; }}}}}}}}}}}}}}}}
    // if no matches, then return a 0
    return 0;
}

The comparearrays routine is as follows:

int comparearrays(int* a1, const uint8_t* a2, int positions) {
  for ( int tmp = 0; tmp<positions; tmp++ ) {
    if ( a1[tmp] != a2[tmp] ) { return 0; }
  } 
  return 1;
}

I'm having a little difficulty getting PROGMEM to work as I need it to but thought I would ask this question first, in case there is an even better way to do this. I would love something along the lines of:

 if ( comparearrays (incoming, [0,0,21,0,8,80,3,65,76,4,0,1,0],    12) == 1 ) { lastRadioRead = 0; return  15; } else {

but have not been able to provide on-the-fly integer arrays for this type of comparison. I was able to do a similar thing with char arrays somewhere else in my code by putting the array info in quotes, but it seems an integer array is either different syntax or not supported.

Any suggestions from you coding veterans?