Logging a perennial memory issue

[maxieds]

Logging a perennial memory issue

[dev-zzo]

Two things immediately stand out to me...

• Check stack usage, as you're allocating a buffer on the stack you might exhaust available stack space.
• You subtract block size from numBlocks, which is counter-intuitive given the variable name; check you actually use it correctly or make sure it is always aligned to block size otherwise you WILL miss your loop condition and loop forever as numBlocks is unsigned. Also see you use MIN when calling MemoryWriteBlock but not when subtracting.

[dev-zzo]

i dunno, try printing out the arguments you're passing and see if they make any sense at all? then capture bus traffic and see if that provides any insight?

[ceres-c]

Have you tried to change the BASISTR ISR to maybe blink a led or something like that? It could help you to determine wether it's a stack corruption issue or due to never reaching the loop exit condition.

[ceres-c]

I don't think that's possible. Those macro are expanded by the preprocessor at compile time, given the ISR will be called at unexpected times it does not seem possible to me to access those values

[maxieds]

@dev-zzo
Yes, you are right about the variable naming being off for the third parameter above. Must have not had enough coffee, or been staring at too much code, when I wrote that down. I should have enough space on the stack for 32 bytes, but not necessarily much more, which is why I'm looping over the FRAM space to initialize things only this minimally small size at a time. The idea is to initialize a chunk of allocated FRAm real estate to a constant byte value across the space (analogous to the standard C-string function memset).

The function name is indicative of what it should be doing. Based on my read of the function MemoryWriteBlock and what it calls to write the FRAM in Memory.c I think that subtracting the block size is still correct usage since that function should be the number of bytes to write.

Now that you mention it, I am usually writing block-wise jagged data. So the issue is probably the subtraction without a check for unsigned arithmetic over/underflow. Thank you for pointing that out. It really helps to have a second set of eyes on the code! My proposed fix is as follows:

void MemsetBlockBytes(uint8_t initValue, SIZET startBlock, SIZET byteCount) {
    BYTE fillerBuf[DESFIRE_EEPROM_BLOCK_SIZE];
    memset(fillerBuf, initValue, DESFIRE_EEPROM_BLOCK_SIZE);
    SIZET writeAddr = startBlock * DESFIRE_EEPROM_BLOCK_SIZE;
    while(byteCount > 0) {
        MemoryWriteBlock(fillerBuf, writeAddr, MIN(DESFIRE_EEPROM_BLOCK_SIZE, byteCount));
        writeAddr += DESFIRE_EEPROM_BLOCK_SIZE / sizeof(SIZET);
        if(byteCount > DESFIRE_EEPROM_BLOCK_SIZE) {
           byteCount -= DESFIRE_EEPROM_BLOCK_SIZE;
        }
        else {
            break;
        }
    }
}

I will try testing this out later when I circle back to the firmware project and testing.

[maxieds]

I changed the implementation to the following:

/* TODO: Why doesn't this work ??? -- It freezes the AVR chip when run !! */
void MemsetBlockBytes(uint8_t initValue, SIZET startBlock, SIZET byteCount) {
    BYTE fillerBuf[DESFIRE_EEPROM_BLOCK_SIZE];
    memset(fillerBuf, initValue, DESFIRE_EEPROM_BLOCK_SIZE);
    SIZET writeAddr = startBlock;
    while(byteCount > 0) {
        WriteBlockBytes(fillerBuf, writeAddr, MIN(DESFIRE_EEPROM_BLOCK_SIZE, byteCount));
        ++writeAddr;
        if(byteCount > DESFIRE_EEPROM_BLOCK_SIZE) {
            byteCount -= DESFIRE_EEPROM_BLOCK_SIZE;
        }
        else {
            break;
        }
    }
}

It still doesn't seem to be initializing things in FRAM correctly ...

[maxieds]

I thought it might be something weird with the uint16_t addresses not providing full 32-bit addresses into the FRAM. Using block sizes of 16 just confuses things and causes other unexpected side effects. I thought you might have some suggestions. In your original code the ReadBlockBytes and WriteBlockBytes refer to EEPROM's number of bytes per block. Based on my inspection of the Memory.c code, it is now FRAM that can get stored back into FLASH from time to time. Maybe somethings have changed in the firmware since that code was written?

[maxieds]

I also do not know how to fix this. Pretty much stuck for a while. This only seems to be relevant when writing and initializing the file system commands. Everywhere else in the code, this seems to be working correctly based on preliminary testing.

[maxieds]

@ceres-c
I didn't know about that debugging option. I will have to try it out if problems persist in the code. My tests today suggest that making the change from 32 byte block sizes to 16 byte block sizes when addressing the data written out to FRAM (as I mentioned above for a possible cause), seems to fix the problems I was having. Thanks for the suggestion. I will keep it in mind.

[maxieds]

@ceres-c
Do you know how to save state for the __LINE__/__FILE__/__func__ macros that are current just before jumping into the BASISTR ISR? That would be very helpful to figure out if resetting the block size to 16 for FRAM memory reads/writes actually works. There seems to be very little documentation on the search engines about this.

[maxieds]

@ceres-c
I guess that makes sense. If you have to stash a copy of those values for every roughly "atomic chunk" of code that gets run to see what is crashing things, the firmware will probably just thrash most of the time even when it is working well. This goes back to my very much wanting to have a gdb-like interface and set breakpoints, etc. to figure out problems.

Just FYI, the reason I asked about something like this is that when I have debugged some of the DESFire related memory routines with my CMLD app, there is a need to store the (say), __LINE__ and __FILE__ associated with the call to WriteBlockBytes. Otherwise, with all of the calls to that function, you get no meaningful output just knowing that an exceptional case happened. For example, something like the following macro setup works well in that instance:

void WriteBlockBytesMain(const void *Buffer, SIZET StartBlock, SIZET Count);
#define WriteBlockBytes(Buffer, StartBlock, Count)              ({ \
    snprintf_P(__InternalStringBuffer2, DATA_BUFFER_SIZE_SMALL,    \
             PSTR("%s @ %d"), __func__, __LINE__);                 \
    __InternalStringBuffer2[DATA_BUFFER_SIZE_SMALL - 1] = '\0';    \
    WriteBlockBytesMain(Buffer, StartBlock, Count);                \
    })

[ceres-c]

I guess you could use a global string, update it accordingly everywhere you need it and print in the BADISTR ISR. But I'm not sure you'd be able to, since, once you land there, the stack is toast and you probably lost the USB altogether...

[maxieds]

@ceres-c
Hmm. So there's not a way to have the compiler detect that a violation or bad memory access has happened and give the error handling functions a chance to react without obliterating the stack?

I was reading some documentation about avr-gcc compiler flags. It looks like they have the standard, non-embedded memory protection options -fsanitize=address (see this doc). I don't know how this would get handled with an AVR that does not easily print or SEGFAULT out to console. I have used these debugging flags before in PC-Linux/Mac software. The behavior there is to exit with a detailed warning and stack trace to the offending line of code printed to the console.