Using sprintf with re-usable buffer - reliability issues

Projects Programming
1

Happy to post full sketch, although it's 1300+ lines, so maybe easier if I explain my problem with snippets?

I have a Mega 2560 sketch that sends and receives messages over MQTT.

There's a callback function which, when MQTT client receives an incoming message, it checks to see what topic the message is on, before assigning e.g. a command, setting, or value to an Arduino variable, e.g. character array.

e.g. when the audio track changes, this callback writes the track name to a char array:

#define trackLength 30     // Now Playing track: max length to accept / display onto Arduino

char track[trackLength+1];

char buff[43]; // general buffer. char array. Assuming topicroot is max 10 chars with null terminator
               // this should never ever exceed 43. #sram

void callback(char* topic, byte* payload, unsigned int length) {
  payload[length] = '\0';

  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, audiovector, titlevector);
  if(strcmp(topic, buff) == 0)
  {
    for (int i=0; i<trackLength+1; ++i)
    track[i] = payload[i];
  }

  [...] other settings / values written in this function

}

A while ago I ran into issues caused by hitting the 8k SRAM limit, but reduced SRAM usage by 60% by simply being more careful with char array sizes, more judicious use of structs, using defines instead of variables where poss (e.g. see above), and also by setting up this re-usable global buffer (char buff[43]).

I started to use this buffer for other things in the sketch, e.g. writing to the display:

      for (int i = 0; i < circuitCount; i++) {
        sprintf (buff, "%i", lightCircuit[i].level);
        u8g.drawStr (circuit_display_x_coord+(circuitSpacing/2) - (u8g.getStrWidth(buff)/2), minor_text_y_coord, buff);
        if (i == currentCircuit-1) { // indicate current circuit being controlled
          // dot -- u8g.drawStr (circuit_display_x_coord+(circuitSpacing/2) - (u8g.getStrWidth(buff)/2) - 7, minor_text_y_coord-3, ".");
          u8g.drawLine(circuit_display_x_coord+(circuitSpacing/2) - (u8g.getStrWidth(buff)/2), minor_text_y_coord+4, circuit_display_x_coord+(circuitSpacing/2) + (u8g.getStrWidth(buff)/2) -2, minor_text_y_coord+4);
        }
        // Serial.print("Writing display: circuit value ");
        // Serial.print(i);
        // Serial.print(" ");
        // Serial.println(buff);
        circuit_display_x_coord = circuit_display_x_coord + circuitSpacing;
      }

After working fine over many revisions over the last 6 months, after a code revision yesterday the sketch randomly broke. I traced the problem to some more settings I had added into the Sketch.

In my callback function I previously had this, and it worked fine:

  // Settings
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuitcountvector); // home/bedroom/settings/circuit_count
  if(strcmp(topic, buff) == 0) circuitCount = atoi( (const char*) payload );
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_1_namevector); // home/bedroom/settings/circuit_1_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[0].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_2_namevector); // home/bedroom/settings/circuit_2_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[1].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_3_namevector); // home/bedroom/settings/circuit_3_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[2].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_4_namevector); // home/bedroom/settings/circuit_4_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[3].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_5_namevector); // home/bedroom/settings/circuit_5_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[4].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_6_namevector); // home/bedroom/settings/circuit_6_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[5].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, light_scene_1_namevector); // home/bedroom/settings/light_scene_1_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightScene[0].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, light_scene_2_namevector); // home/bedroom/settings/light_scene_2_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightScene[1].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, light_scene_3_namevector); // home/bedroom/settings/light_scene_3_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightScene[2].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, light_scene_4_namevector); // home/bedroom/settings/light_scene_4_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightScene[3].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, light_scene_5_namevector); // home/bedroom/settings/light_scene_5_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightScene[4].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, audio_fave_1_namevector); // home/bedroom/settings/audio_fave_1_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) audioFave[0].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, audio_fave_2_namevector); // home/bedroom/settings/audio_fave_2_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) audioFave[1].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, audio_fave_3_namevector); // home/bedroom/settings/audio_fave_3_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) audioFave[2].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, audio_fave_4_namevector); // home/bedroom/settings/audio_fave_4_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) audioFave[3].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, audio_fave_5_namevector); // home/bedroom/settings/audio_fave_5_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) audioFave[4].name[i] = payload[i];

To the end of this I added pretty much more of the same:

  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, heat_fave_1_namevector); // home/bedroom/settings/heat_fave_1_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) heatingProfile[0].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, heat_fave_2_namevector); // home/bedroom/settings/heat_fave_2_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) heatingProfile[1].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, heat_fave_3_namevector); // home/bedroom/settings/heat_fave_3_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) heatingProfile[2].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, heat_fave_4_namevector); // home/bedroom/settings/heat_fave_4_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) heatingProfile[3].name[i] = payload[i];
  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, heat_fave_5_namevector); // home/bedroom/settings/heat_fave_5_name
  if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) heatingProfile[4].name[i] = payload[i];

... and it broke the functionality of my sketch. I mean, the sketch still runs, but some other areas of the sketch, e.g. pressing buttons to send MQTT messages, displaying the screen output, and more.

I know you probably need to see the code for the bits of the functionality that have broken, but basically can someone advise me on whether it's a very silly idea to use a global buffer variable in this way, or provide any other ideas?

2

sprintf_P would be a better choice.

// Serial.print("Writing display: circuit value ");

I know it's commented-out, but it worries me that other such prints are so profligate.

3

Without seeing the whole code I can not make good suggestions,
but the posted snippets look absolutely unmaintainable, copy-and-pasted. :frowning:

4 
char buff[43]; // general buffer. char array. Assuming topicroot is max 10 chars with null terminator
               // this should never ever exceed 43. #sram

Based on the comment and the surrounding code the buffer needs to be 44 bytes not 43.

However, that problem is very likely the proverbial drop-in-the-ocean. @AWOL pointed you to the ocean.

5

@AWOL - thanks for the suggestion. (There are no other Serial.prints - at least not that haven't been commented out...)

@Whandall - why un-maintainable?

@Coding Badly - my calculation of 43 took account of null terminator...

6

To me it looks like a compact block of the same stuff copied over and over,
with very little differences. I would try to factor out the common stuff,
but how that can be done, needs the knowledge of all the declarations etc.

A first step could be to use functions, for example

if(strcmp(topic, buff) == 0) for (int i=0; i<nameLength+1; ++i) lightCircuit[2].name[i] = payload[i];

if (!strcmp(topic, buff)) {
  memcpy(lightCircuit[2].name, payload, nameLength+1);
}

and because this is repeated over and over, I would move that into a function.

  sprintf (buff, "%s/%s/%s/%s", rootvector, zonevect, settingsvector, circuit_1_namevector); // home/bedroom/settings/circuit_1_name

  sprintf (buff, "%s/%s/%s/", rootvector, zonevect, settingsvector); // only once

  strcat(buff, circuit_1_namevector); // later on

If your snippets are not even snippets, but grep output, I can not say much at all.

7

@Whandall - I see what you mean now. There's so much optimisation to be done on my code, which I agree is as messy if not more so than my bedroom. Thanks for the tips. Really appreciate them.

Re AWOL's suggestion, will sprintf_P save me RAM in the case where the arguments provided to it are actually defines (i.e. not variables)? When do the values in question get copied to RAM?

What I mean is this: of course if I declare rootvector, zonevector, settingsvector, circuit_1_namevector as character arrays, then these are copied to RAM at startup. But if they are defined like this:

#define rootvector "home"

then would I still benefit from sprintf_P?