Code help please - API calls crashing core

Cloud
21

@bko @ScruffR , so finally I am “on-site” with my spark! What I’m getting is the red flashes (… — …) followed by one slow red flash. Then the sequence repeats. I’m getting this even with just the spark plugged into a USB port for power with nothing else attached (no LEDs etc.) This seems to equate to “Hard Fault”

Since @ScruffR didn’t have this problem, does this mean I have a faulty core?!

@ScruffR I wonder if you are using the exact same code as me, and whether you are sending the same API request. Just to confirm the code I am using at the moment is: http://pastebin.com/A2WSHVTa

The libraries are:-

http://pastebin.com/0K63U0nF (LPD6803.cpp)
http://pastebin.com/Fnuwuuer (LPD6803.h)

And I’m using SparkIntervalTimer.h as found in the cloud.

I’m sending the request like this:-

curl https://api.spark.io/v1/devices/jjcore1/ledSet -d access_token=xxxxxxxxxx -d “c=5-32-32-32-”

Many thanks for your continued advice.

22

Hi @jellifish

I think your program should be written a lot more defensively to guard for errors. In particular, String.indexOf(char) can return -1 if the char is not found and then toInt() returns zero if there are no numbers in the string, so racknum-1 can be -1 in your code potentially writing some other variable in memory.

So it would be better if you checked for the index == -1 and skipped the update etc. for all the indexOf() and toInt() calls. Be careful any place you write memory.

Some of the problems come from the fact that you have mixed 0-based and 1-based indexing. It might simplify the code if you decide to do all 0-based indices. For instance, the rackWipe() function–I think you are doing the index math correctly but if it was 0-based, it would be easier to understand.

Looks like a cool project!

23

Another point migh be your use of - as a separator in your parameter string.
Since String.toInt() will accept a minus sign as part of your number, you might end up using a negative - or if using unsigned int, hugely positive - index which might again cause some mem corruption.

And as for the index and return value checking suggested by @bko, this was one of my first responses in this thread, too.
If you ask for advice here, it would be nice if it was appreciated (taken up), too. If you don’t you might get the same suggestions over and over again, even if these are not your actual problem, people will always notice the lack of these crucial points first and may stop looking deeper.

24

Hi,

I fully appreciate what you are saying here, and I am certainly understanding the value of the advice and importance of bounds checking and value checking etc. I suppose I just wanted to keep the code as simple as possible at this stage to try and better pinpoint where it might be going wrong.

Just to re-iterate, the code works as it is and as expected (with API calls etc.) simply by commenting out the strip.begin(); strip.show(); lines.

Am I wrong in thinking therefore that this must relate to the library itself (which @peekay123 kindly helped port ) and its interaction with API calls and not have much to do with my code?

To further clarify, even if the API function does nothing except return 1 it will still crash in the same manner:-

int ledSet(String c) {

return 1;

}

25

OK it sounds like a memory problem in the library but please don’t forget you titled this thread “API calls crashing core” so we looked at the API calls. All of the previously identified potential problems are real potential problems; they just are not the problem you are currently having.

Maybe @peekay123 can have another look at the library with an eye for memory/pointer problems.

26

Hi,

Yes, agreed there are lots of potential problems in my code!

I’ve tried to simplify the code down to the barest possible to demonstrate the problem now, and I have ended up with this:-

http://pastebin.com/SCWnDjpS

Note that with line 27 in, the API call causes the Hard Fault, with line 27 commented out the API call returns 1 as expected. Hopefully @peekay123 can spot something for me :smile:

27

@jellifish, one thing I spotted:

line 27: colorWipe(Color(0,0,0),50);

void colorWipe(uint16_t c, uint8_t wait)

unsigned int Color(byte r, byte g, byte b)

So colorWipe() expects a uint16_t as the first argument but Color() returns an unsigned int. Color() should return a unint16_t.

If the LED library works fine without the Spark.function() call then it is not the library. When you say “the API call causes a Hard Fault”, do you mean when you call Spark.function()? So if colorWipe() is NOT called in setup(), calling Spark.function() works fine. BUT if colorWipe() is kept in setup(), calling Spark.function() causes a hard fault?

28

Hi again @peekay123 ,

First let me say, I didn’t write any of the code (colorWipe or Color functions) these were just samples with the original library. I have to confess I find the whole storing of the RGB values in a single value a little confusing, but as you say, the LED library does work fine without the Spark.function() call. Are you suggesting I change colorWipe or Color in terms of the variable type?

You are also correct that when colorWipe is NOT called (ie. commented out) calling Spark.function works exactly as it should. It seems to me there’s some conflict between the two libraries (the API one and the LED one)? Does that even make any sense?

Really appreciate your continued interest in this :slight_smile: I’m really just a perl programmer who finds memory management and hard faults all quite scary :wink:

29

@jellifish, I’ll try and flash the firmware to a Core and take a look. :smile:

30

@jellifish, I played with this last night and got very odd results. If in your simplified app I replace the “i” in line 47, colorWipe(), with a fixed number, say zero, then calling Spark.function() works just fine. Putting “i” back and calling Spark.function() causes the crash. I am trying to figure out why populating the led array with values in setup() causes a call to Spark.function() to crash the Core. I now have to dig into the LPD6803 code to get some insight but right now, I am stumped.

@bko, any ideas or guidance? I will disable the timer interrupts just to see if there is conflict there. However, the Torch code uses a timer-based interrupt as well and that works without a hitch. I wonder if this is caused by some compiler voodoo. :smile:

31

Just a quick theory (not checked any further):

In LPD6803.cpp

LPD6803::LPD6803(uint16_t n, uint8_t dpin, uint8_t cpin) {
  dataPin = dpin;
  clockPin = cpin;
  numLEDs = n;
 
  pixels = (uint16_t *)malloc(numLEDs);  // <-- possible indices (0 .. numLEDs - 1)
  for (uint16_t i=0; i< numLEDs; i++) {
    setPixelColor(i, 0, 0, 0);
  }
 
  SendMode = START;
  BitCount = LedIndex = BlankCounter = 0;
  cpumax = 100;
}

...

void LPD6803::setPixelColor(uint16_t n, uint8_t r, uint8_t g, uint8_t b) {
  uint16_t data;
       
  if (n > numLEDs) return;
 
  data = g & 0x1F;
  data <<= 5;
  data |= b & 0x1F;
  data <<= 5;
  data |= r & 0x1F;
  data |= 0x8000;
 
  pixels[n] = data;
}

void LPD6803::setPixelColor(uint16_t n, uint16_t c) {
  if (n > numLEDs) return;
 
  pixels[n] = 0x8000 | c;
}

Should if (n > numLEDs) return; not rather be if (n >= numLEDs) return?

But since @jellifish does this

  ...
  for (i=0; i < strip.numPixels(); i++) {
      strip.setPixelColor(i, c);
      strip.show();
      delay(wait);
  }
...

the above shouldn’t matter

32

This is unusual in that is allocates numLEDs bytes and then says to pretend they are 16-bit unsigned ints. Are there supposed to numLEDs bytes or numLEDs uint16_t?

A more usual way would be to multiply the number of items you want by the sizeof() the item in bytes.

33

That’s another thing I did miss - good catch :+1:

34

@bko I’d say you are right. The malloc returned address of the buffer is stored in variable pixel as a uint16_t pointer. setPixelColor will format and store the variable data into the pixel array with the subscript n. data is also uint16_t and n will be a 16 bit subscript. So this will be clobbering someone else’s memory once n is halfway thru the led range. So it should change to malloc ((numLEDS << 1).

@ScruffR - You are also right. if (n >= numLEDs ) return; is needed otherwise it can still clobber memory if n is equal to numLEDs.

35

@ScruffR, @bko, @pra, nice catch! I’ll make some mods tonight and run some tests but that has to be the issue. :smile:

UPDATE: That was the problem! I fixed the malloc() call and all worked fine!

@jellifish, I have fixed the problem in the repo library file :slight_smile:

36

Woohoo! Going to try this tonight :slight_smile: Thank you :smile:

37

Hi,

So the good news is it’s now not crashing so from that point of view I guess this thread is completed.

The bad news is, the LED string is still not doing what I expect always (but does sometimes). In particular the colours just don’t seem to work beyond being able to be set to 0 (off) 32 (dim) and 64 (fully bright), I can’t seem to get any gradation beyond that. I don’t know if that’s a fault with my hardware or with the library.

In addition, for some reason if I set the first LED (which in my case is actually three, because the strip counts each pixel as 3, a hardware quirk) to zero (ie. off) the strip just lights up all over in a random way. I can set it to a colour without trouble. Again I don’t know if this is a library issue or a hardware issue.

So, as I say, since this topic was about API calls crashing I guess for neatness this thread should close, but I don’t know if @peekay123 you are interested in having any further look at this library (which isn’t yours and may well be buggy to start with)?

Thank you one and all again, and at least where I am now, I can do something useful (if a bit more limited than I wanted) so all is not completely lost from my perspective.

38

@jellifish, can you post your app? Have you run the strandtest.ino which comes with the library? That runs a rainbow and colorwheel which should show off the color gradient. As for the first LED, I will have to look at the library some more once I understand what your app is doing.