Logging a perennial memory issue

[maxieds]

Perhaps @dev-zzo can shed some light into this. I had to remove some similar code from his original repo sources to get things working correctly. I have a stub (with comments) for this point. Basically, I do not exactly understand why running the following code to initialize a new chunk of FRAM real estate to zero (or whatever it should be) freezes the AVR:

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

My best estimate is that things need to be aligned along block (defined at 32 bytes here) sizes. But then this can cause many an issue with having to write jagged data.

Why doesn't this work? I guess I should tag/ask @ceres-c too since he does so much of the development and PR merging over at emsec. A solution to this may very well stabilize some code that has gone a awry so far in my sources!

Thanks for the input. :)

[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.

[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 ...

[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?

[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.

[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.

[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.

[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]

@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.