Teensy 4.1 crash, using ILI9341 and ArduinoJson

Status
Not open for further replies.

graydetroit

Well-known member
Hi,

My project is a MIDI sequencer, and I'm using two Teensy 4.1's. Here's a picture of the unit:

Screen Shot 2020-12-27 at 6.31.27 PM.jpg

One Teensy handles the MIDI sequencer programming, MIDI output, USB host devices, and the button input processing.

The second Teensy handles updating the ILI9341 display. I did this because the display processing was often causing the MIDI clock to stutter, and I didn't want to mess around with threading. DMA didn't seem to help.

I'm using the ArduinoJson library to marshal my custom classes/data structures to serialized JSON, and to send them over the hardware Serial5 port to the second Teensy, and then the second Teensy deserializes and unmarshals the JSON back to the original classes/data structures to be used for display processing.

The problem is that after a certain amount of display updates with a JSON payload of around 3-5KB, the second Teensy freezes/crashes, presumably because there's a stack overflow or something like that. Here's an example JSON payload that the first Teensy sends to the second.

Here's the code. I'm sure I'm doing something extremely inefficient, and I'd be so appreciative if you can let me know how I can improve the code so that the memory usage doesn't become a problem so easily.

I tried using DMAMEM and I have the 8MB PSRAM chip on the second Teensy and I've tried initializing the dynamic JSON document buffer with EXTMEM, but nothing seems to work, I'm at my wit's end.

I can record a video of the behavior if that would help also.

Thanks so much in advance.
 
could it be possible the dynamic memory allocation "new" being called in the loop in multiple functions (also within a for loop) may be eating up the ram? have you checked if the heap is decreasing?
 
Sometimes it is hard to debug some of these types of crashes/hangs.

If it were me with this setup and issue. I would try adding a bunch of debug stuff in. Also don't know how long of a run you have before it crashes.

Some of the stuff I would typically do, is to use several IO pins that I either set high/low around pieces of code and/or toggle at key points and setup logic analyzer on it, such that when it dies or hangs, maybe you will see the state of where it is happening...

I also use lots of Serial.print... althought sometimes this changes timing which masks the issue.

If I was concerned about running out of memory and the like, I then maybe hack in some code to try to estimate stack usage and Heap usage...

There is some of this we did into one of the ST7735(ST7789)_t3 example code (uncannyeyes_async_st...).

I think someone like @Frank B has a library that does some of this?
But the code at setup tries to set the area of the end of memory up to the current stack down to the last used memory with some known value, and then in the loop it scans to see where is the last place that we still have that known value to guess how far the stack may have grown.

I don't remember if this one showed the malloc stuff
But if not I typically just see how far it extended by doing something like:

Code:
extern "C" {
void * _sbrk(int incr)
}
...
void loop() {
  uint32_t current_heap = (uint32_t)_sbrk(0);
}
And see if it continues to grow ...
 
In regards to the heap, @KurtE and @tonton81, yes it appears to be growing. Using Kurt's method above, I have an initial heap value logged at 539152384, and then right before it crashes there's a heap value logged at 539480064 -- it grew by 327680.

I'm also using `delete` on the structures that i'm instantiating with `new`, but maybe I'm not doing something right given that they're pointers?
 
Last edited:
Ah, I was able to eke out a bit more memory by also deleting the object instantiations I was doing in nested for loops. Maybe I assumed calling `delete` on a parent object which has several child objects instantiated with `new` also gets its memory freed up.
 
There are very few circumstances where you should be using “new” in modern C++.
The code you linked to uses “new” and owning raw pointers, and leaks memory as a result.

For example here:
Code:
std::vector<SequenceInstrumentTrackProgram *> instrumentTrackPrograms;
auto *instrumentTrackProgram = new SequenceInstrumentTrackProgram(
    instrumentTrackObj["tIndex"],
    prIdx
);
instrumentTrackProgram->trackIndex = instrumentTrackProgramObj["trackIndex"];
instrumentTrackPrograms.push_back(instrumentTrackProgram);

There's no reason to dynamically allocate the elements of your vector using “new”, vector already allocates space for you to use. A better alternative would be:
Code:
 // vector of objects, not pointers to objects
std::vector<SequenceInstrumentTrackProgram> instrumentTrackPrograms;
// construct the object into the vector directly
instrumentTrackPrograms.emplace_back(instrumentTrackObj["tIndex"], prIdx); 
// get a reference to the new object
auto &instrumentTrackProgram = instrumentTrackPrograms.back(); 
// use the reference to update some of the members
instrumentTrackProgram.trackIndex = instrumentTrackProgramObj["trackIndex"];

Similarly, here:
Code:
SequenceInstrumentTrackProgram* unmarshalInstrumentTrackProgram()
{
    auto instrumentTrackProgram = new SequenceInstrumentTrackProgram(
        doc["trackIdx"].as<unsigned int>(),
        doc["prgIdx"].as<unsigned int>()
    );
    // ...
    return instrumentTrackProgram;
}
You should never return the result of “new”, this is guaranteed to result in memory leaks.
Always prefer returning objects:
Code:
SequenceInstrumentTrackProgram unmarshalInstrumentTrackProgram() // returns an object, not an owning pointer
{
    SequenceInstrumentTrackProgram instrumentTrackProgram{
        doc["trackIdx"].as<unsigned int>(),
        doc["prgIdx"].as<unsigned int>()
    };
    // ...
    return instrumentTrackProgram; // return by value
}
In the rare case where you really need reference/pointer semantics (e.g. when returning abstract or polymorphic types), always use smart pointers, never transfer ownership using raw pointers:
Code:
std::unique_ptr<SequenceInstrumentTrackProgram> unmarshalInstrumentTrackProgram() // returns an object, not an owning pointer
{
    auto instrumentTrackProgram = std::make_unique<SequenceInstrumentTrackProgram>(
        doc["trackIdx"].as<unsigned int>(),
        doc["prgIdx"].as<unsigned int>()
    );
    // ...
    return instrumentTrackProgram;
}
You can find some more information in the C++ Core Guidelines if you're interested: http://isocpp.github.io/CppCoreGuid...r-ownership-by-a-raw-pointer-t-or-reference-t

Pieter
 
Wow, thank you so much! I'm not an experienced embedded developer, and as such I'm less familiar with good design patterns and how to manage memory properly, so this is very helpful.
 
Status
Not open for further replies.
Back
Top