weird weird weird

snowsh

snowsh

Well-known member
I have this section of code, please don't ask for the full codebase, its enormous.

C++: 
  if (checkButtonPress(BUTTON_DRUMMER_FILLS_ENABLE))                                // this works fine
  {
    Serial.println(F("Button press detected: BUTTON_DRUMMER_FILLS_ENABLE"));

    // Debug before toggling the fill enable state
    Serial.print(F("Current fill enable state: "));
    Serial.println(this->config.enableFills  ? "on" : "off");
    Serial.println(this->config.enableFills);

    // some weird fucked up shit going on here...
    // Toggle the fill enable state - this->config.enableFills is a bool!
    //this->config.enableFills = !this->config.enableFills;                         // toggles betwen 4 and 5?????????

    if (this->config.enableFills > 0) this->config.enableFills = false;
    else if (this->config.enableFills == 0) this->config.enableFills = true;

    // Debug after toggling the fill enable state
    Serial.print(F("New fill enable state: "));
    Serial.println(this->config.enableFills ? "on" : "off");
    Serial.println(this->config.enableFills);

    // Update button lights
    setButtonLights();                                                              // this works fine
  }


this->config.enableFills is declared as a bool.

When I don't have an SD card present, here is my debug:
Code: 
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
1
New fill enable state: off
0
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: off
0
New fill enable state: on
1
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
1
New fill enable state: off
0
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: off
0
New fill enable state: on
1
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
1
New fill enable state: off
0

now when I have an SD card present:
Code: 
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
4
New fill enable state: on
5
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
5
New fill enable state: on
4
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
4
New fill enable state: on
5
Button press detected: BUTTON_DRUMMER_FILLS_ENABLE
Current fill enable state: on
5
New fill enable state: on
4

I have tried everything. Even declaring the value in a different location in case it was some weird memory issue:

C++: 
struct Config{
  etc.......
  uint8_t activeRuleId;                                                                           // to track the current fill rule being played
  bool corrupt = false;                                                                           // this was not working. 4/5 id SD present....to enable / disable the use of fills.
  bool enableFills = false;                                                                       // still not right.....    to enable / disable the use of fills.
  byte unused[43];//41];
};

This bool is only referenced 3 times in the codebase outside of the code above, it only reads the var:

C++: 
if (this->config.enableFills)

the only place this value is set is in the button read code above, and at the start of the program in setup() where the SD card is read to populate values. You can see in the debug that the values are f$%^£d the moment the button routine is called.....

This has been doing my head in now for too long. ChatGPT was typically a waste of time....

Anyone have any insights?
 
When you say "an SD card is present"... is this config struct being read from a file on the SD card?
 
Suggest looking at where config.enableFills is defined, then look at whatever is defined/allocated right above it in the definition. With unexpected values like 4 & 5, one strong possibility is that whatever is right above config.enableFills in memory is getting overrun & these values are spilling into this storage space.

Another approach is to define an array right of some number of bytes (16, 32, 54, etc.) above config.enableFills and fill this array with repeating pattern of 0xaa 0x55, then periodically read this array to see if anything ever changes unexpectedly.

Good luck !!

Mark J Culross
KD5RXT
 
hmmmm.... if i place this at the end of setup():


Code: 
  drummer.config.enableFills = false;

saveHandlerToSD(SD_TYPE_CONFIG, 0);

it works, even with the SD card present.
 
ss-bool-debug-cfgData-dump.PNG

here is my struct definition:
Code: 
struct DrummerConfig {
  uint8_t channel;
  uint8_t screenMode;
  uint8_t midiOutChannel;
  uint8_t midiInChannel;
  uint8_t stepLength;
  uint8_t kitId;
  uint8_t currentPatternId;
  char kitName[CHANNEL_NAME_LENGTH];
  int kitColors[4];
  int selKitColorHit;
  int selKitColorAccent;
  int kitColorHit;
  int kitColorAccent;
  int kitColorHighlight;
  uint8_t numberOfInstruments;
  char instrumentNames[NUMBER_OF_DRUMMER_INSTRUMENTS][DRUMMER_INSTRUMENT_NAME_LENGTH];
  uint8_t instrumentNoteNumbersAr[NUMBER_OF_DRUMMER_INSTRUMENTS];
  uint8_t instrumentMidiOutChannel[NUMBER_OF_DRUMMER_INSTRUMENTS];
  uint8_t instrumentVelocity[NUMBER_OF_DRUMMER_INSTRUMENTS];                                      // set low to allow for accents
  uint8_t instrumentStepLength[NUMBER_OF_DRUMMER_INSTRUMENTS];
  uint8_t instrumentResolution[NUMBER_OF_DRUMMER_INSTRUMENTS];
  int8_t instrumentPosition[NUMBER_OF_DRUMMER_INSTRUMENTS];
  bool instrumentPlayed[NUMBER_OF_DRUMMER_INSTRUMENTS];
  bool instrumentMute[NUMBER_OF_DRUMMER_INSTRUMENTS];                                             // true = muted
  uint8_t probability[NUMBER_OF_DRUMMER_INSTRUMENTS];                                             // global: 0 = never play, 100 = always play, 101 = read pattern probabilities
  uint8_t currentFillRuleSet = 0;                                                                 // to determine which fill libray rule set we are editing
  uint8_t currentFillRuleId = 0;                                                                  // to determine which fill rule id we are editing
  uint8_t activeRuleId;                                                                           // to track the current fill rule being played
  bool corrupt = false;                                                                           // to enable / disable the use of fills.
  bool enableFills = false;                                                                       // to enable / disable the use of fills.
  byte unused[43];//41];
};

I'm beginning to wonder if the problem is the data in the SD card......
 
I'd say it's a problem with how you're filling the struct with data from the SD card.
It's not safe to just take the pointer of a struct and access it like an array; there may be padding bytes between members, and the format will shift around if you ever add/remove more fields.
 
well, that seems to have found it!

here is the intialised SD data:
ss-bool-debug-cfgData-dumpFreshStruct.PNG


and having reworked the values:


ss-bool-debug-cfgData-dumpNew.PNG
 
jmarsh said:
I'd say it's a problem with how you're filling the struct with data from the SD card.
It's not safe to just take the pointer of a struct and access it like an array; there may be padding bytes between members, and the format will shift around if you ever add/remove more fields.
Click to expand...
Yes, I'm aware of the issues with changing the structures. I have gone with this because I need it to be as fast as possible. I had experimented with JSON but there was too much overhead and it became unworkable due to the real-time nature of this project (music sequencer). CSV would have similar issues - dealing with strings etc 🤔

Storing the structs in this way has proven to be fast, and to date 100% reliable. I suspect the issue is I had refactored the struct and removed some things and increased the unused array size, then I added the bool. There must have been some bytes left over in the SD struct, but to be honest I didn't expect this. It's so low level the SD doesn't know anything other than 1s and 0s, same with the data exchange in the program. Looks like I found the exception today! Its most odd because the data should have simply been overwritten any time I make a change to these values in the UI. I usually only add to the structs hence the byte unused[x] array at the end of my struct declarations for exactly this reason. For the past (ahem) years of development, its never been an issue. If the struct grows beyond the available size, I simply delete the file on the SD card and the setup process takes care of creating a new file. I will need to devise something however for future upgrades, I was thinking of an online upgrade program...

Anyway, I am glad the issue is found and fixed. If anyone has more insight to offer with this approach of storing raw structured data to SD I am keen to discuss!
 
Well if you're set on just dumping the raw struct contents to disk...
I would suggest putting a version field at the start of the struct, and name the struct accordingly (e.g. ConfigV1). Then any time you change the layout, make it a new definition (ConfigV2, ConfigV3...). Make the load function read the version from the SD file first so it knows what type of config is stored on it, load it into a config struct matching that version, then copy the relevant fields from the "old" config struct definition into the current one. Any fields that can't be copied from the old struct (because they didn't exist) should be manually initialized to defaults.
 
jmarsh said:
Well if you're set on just dumping the raw struct contents to disk...
I would suggest putting a version field at the start of the struct, and name the struct accordingly (e.g. ConfigV1). Then any time you change the layout, make it a new definition (ConfigV2, ConfigV3...). Make the load function read the version from the SD file first so it knows what type of config is stored on it, load it into a config struct matching that version, then copy the relevant fields from the "old" config struct definition into the current one. Any fields that can't be copied from the old struct (because they didn't exist) should be manually initialized to defaults.
Click to expand...
This is kind of my approach so far. I have some upgrade methods if I ever need to retain the SD data. I can take care of the naming within the code, the SD files are already named based on their counterpart struct, albeit with a 3-letter code. I like the idea of adding a version number to the start of the struct. I could keep that standard as simply an int. It's these details that take time..... Almost there!
The issue with doing this in program is all these struct definitions are going to become a mare, and I have already maxed out memory many times and had to do allot of work to regain space.
 
snowsh said:
Yes, I'm aware of the issues with changing the structures. I have gone with this because I need it to be as fast as possible. I had experimented with JSON but there was too much overhead and it became unworkable due to the real-time nature of this project (music sequencer). CSV would have similar issues - dealing with strings etc 🤔

Storing the structs in this way has proven to be fast, and to date 100% reliable. I suspect the issue is I had refactored the struct and removed some things and increased the unused array size, then I added the bool. There must have been some bytes left over in the SD struct, but to be honest I didn't expect this. It's so low level the SD doesn't know anything other than 1s and 0s, same with the data exchange in the program. Looks like I found the exception today! Its most odd because the data should have simply been overwritten any time I make a change to these values in the UI. I usually only add to the structs hence the byte unused[x] array at the end of my struct declarations for exactly this reason. For the past (ahem) years of development, its never been an issue. If the struct grows beyond the available size, I simply delete the file on the SD card and the setup process takes care of creating a new file. I will need to devise something however for future upgrades, I was thinking of an online upgrade program...

Anyway, I am glad the issue is found and fixed. If anyone has more insight to offer with this approach of storing raw structured data to SD I am keen to discuss!
Click to expand...

Changing compilers, changing struct member ordering, adding and removing structs, can all mess with the physical structure. Just because it works now, doesn't mean it will work later. At the very least, my suggestion is to use a packed struct. Personally, I never trust struct layouts, so I either just read values into the struct manually or use some format such as protobufs (eg. Nanopb).
 
Packed struct gives dependable binary structure, but it (usually) cause the compiler to generate slow code. Whether that speed difference really matters is a good question.
 
PaulStoffregen said:
Packed struct gives dependable binary structure, but it (usually) cause the compiler to generate slow code. Whether that speed difference really matters is a good question.
Click to expand...
One practice I try to follow when designing a structure is to order the elements by size:

Code: 
struct OrderedValues{
  uint32_t big1;
  uint32_t big2;
  uint16_t middle1;
  int16_t middle2;
  bool goodbad1;
  bool goodbad2;
  uint8_t small1;
  char letter1;
  uint8_t small2;
  char letter2;
  char letter3;
};

If the compiler follows normal rules that align elements based on their size, you will have no padding and sizeof(OrderedValues) will be 19 and match my count. (I sometimes forget the size of an int in Arduino, so I specify the size when I declare them (uint16_t, in32_t, etc.). Note that you can mix up elements of different types, as long as they have the same size.



If you mix up your 1, 2, and 4-byte values in your structures, you are sure to have added padding bytes unless you specify that the structure is packed. Sometimes you can't avoid packing--as in the header for a .BMP file, where there are two characters at the beginning, 'b', 'm' followed by a 32-bit size value.
 
mborgerson said:
If the compiler follows normal rules that align elements based on their size, you will have no padding and sizeof(OrderedValues) will be 19 and match my count.
Click to expand...
That should not be the case.
sizeof() rounds up to account for the required alignment - otherwise if you did something like "malloc(X*sizeof(OrderedValues))" some of them would end up with incorrect alignment. So in this case the size of the struct would be 20.

(An exception to this is when a struct is a member of a larger definition, and you use sizeof() on the member name rather than the struct name.)
 
jmarsh said:
That should not be the case.
sizeof() rounds up to account for the required alignment - otherwise if you did something like "malloc(X*sizeof(OrderedValues))" some of them would end up with incorrect alignment. So in this case the size of the struct would be 20.

(An exception to this is when a struct is a member of a larger definition, and you use sizeof() on the member name rather than the struct name.)
Click to expand...
You're right about that. I think I missed that part in the related article in post #15.

I tested that with some code:

Code: 
struct OrderedValues{
  uint32_t big1;
  uint32_t big2;
  uint16_t middle1;
  int16_t middle2;
  bool goodbad1;
  bool goodbad2;
  uint8_t small1;
  char letter1;
  uint8_t small2;
  char letter2;
  char letter3;
};


#pragma pack (1)
struct PackOrderedValues{
  uint32_t big1;
  uint32_t big2;
  uint16_t middle1;
  int16_t middle2;
  bool goodbad1;
  bool goodbad2;
  uint8_t small1;
  char letter1;
  uint8_t small2;
  char letter2;
  char letter3;
};
void setup() {
  Serial.begin(9600);
  delay(400);
  Serial.printf("Size of OrderedValues     = %lu bytes\n", sizeof(OrderedValues));
  Serial.printf("Size of PackOrderedValues = %lu bytes\n", sizeof(PackOrderedValues));

  void *povptr1 = malloc(sizeof(PackOrderedValues));
  void *povptr2 = malloc(sizeof(PackOrderedValues));
  Serial.printf("povptr1 = %08X   povptr2 = %08X   ", povptr1,povptr2);
  Serial.printf("diff = %ld \n", (uint32_t)povptr2-(uint32_t)povptr1);
}

void loop() {
  // put your main code here, to run repeatedly:

}

Result:

Code: 
Size of OrderedValues     = 20 bytes
Size of PackOrderedValues = 19 bytes
povptr1 = 20204088   povptr2 = 202040A0   diff = 24

So you can get odd-sized structures only if you use #pragma pack (1)? If you dynamically allocate packed structures, malloc() will add padding to make sure the memory used is padded to a pointer-size (4-byte) boundary as there is an internally-used pointer as the first element in the allocated memory. Have I got that right? As a long-time embedded programmer as far back as 6502 and 8051 assembly-language days, I've always avoided malloc() like ticks in Lyme-disease country. Perhaps that's why I haven't yet been bitten by these details.

I was pleased to find that The Lost Art of Structure Packing did point out that ordering your elements by size in structures is a useful way to minimize excessive memory use in padding between elements.

I guess you're never too old to learn something new---sometimes at least three or four times!
 
You must log in or register to reply here.
Back
Top