Uno and ProMicro crashes after config upload once a stepper is defined

[elral]

Once a stepper is defined the Uno and ProMicro crashes after the next or overnext (depends on how many additional devices are defined) config upload. On the Mega it needs ~16 uploads w/o reset in between, so it will be very seldom.

To reproduce it on the ProMicro/Uno, define a stepper and e.g. a button. Upload this config and add an additional device. Upload this config again. Does the uplaod end w/o error, at least the next coming upload will not be successfull.

The reason is, steppers are initialized dynamically. During config upload all devices are resetted and for dynamically initialized devices the used memory (RAM) is freed. Unfortenutley this does not really work like we also saw for the 7Segements before.

Initializing the steppers statically will increase the required memory up to ~98%, but it seems to work stable until you define one LCD. The LCD device is also intialized dynamically so we are running into the same problem. The remaining RAM might not be sufficient to initialize the LCD statically, but needs some more tests.

The only solutions I can imagine for now is to free up the required RAM by e.g. dispensing the config buffer, reduce the number of devices or reduce the size of the config buffer.
Dispensing the config buffer would mean some (bigger?) changes on the connector side, but is the most effective for freeing up the RAM.

Any other proposels are welcome.

[neilenns]

I am able to reproduce this locally. I did some digging online and found code that can report the available memory and largest available memory block on an Arduino.

I added this to the firmware and made a new command called kMemory that reports the values. I updated the desktop app to call this command on GetInfo and after ActivateConfig. I used my pro micro with an LCD display, button, 5 LEDs, and one stepper configured.

At initial startup here's the memory report:

Total available memory: 467	Largest available block: 467	 Fragmentation: 0%.

I went into the modules dialog and re-uploaded the same config. This is the report:

Total available memory: 373	Largest available block: 373	 Fragmentation: 0%.

After that any future uploads result in the crash.

[neilenns]

For comparison here's what my Mega shows at startup when I have a bunch of devices and a stepper configured:

Total available memory: 3531	Largest available block: 3531	 Fragmentation: 0%.

And here's what it looks like after uploading the config again:

Total available memory: 3375	Largest available block: 3375	 Fragmentation: 0%.

[elral]

What I found so far:

1. if you are not using "delete" in the :detach() function, then they do not crash!? What is this??
2. moving the variables from the constructor to an additional :init() function helps also, no crashes anymore. BUT in this case only one stepper can be configured. After configuring the second one the Uno does not start anymore. Why is in this case more memory required than to have the variables in the constructor??

Sebastian proposed to make a reset after uploading the config. I think this would be the best way to have always a "clean" memory.

[elral]

@neilenns , seems that at your ProMicro 107 Bytes getting "lost". ~150 Bytes are required for each stepper if you declare them statically.

Could you send me the link for reporting the used memory? I have also somewhere some code from past days, but I have to search it.
And I added a kind of "debug print out" in the connector (will show up in the debug window) and firmware, maybe we can align this.

[neilenns]

I have an LCD display as well so that might account for the remaining 107 bytes?

I pushed two branches for you: Firmware that adds the kMemory command and desktop app that has a GetMemoryStats() method called on GetInfo and ActivateConfig.

[elral]

That's what I found from the past:
`*

• mem_check.h
• Created: 14.05.2014 12:57:21
  */
  #ifndef MEM_CHECK_H_
  #define MEM_CHECK_H_
  unsigned int get_mem_free (void);
  unsigned short get_mem_unused (void);

#endif /* MEM_CHECK_H_ */

/*
Dabei liefert get_mem_free() den derzeit freien Speicher zwischen Stack
und Heap.
get_mem_unused() liefert die Speichergröße, die noch nicht "angerührt"
wurde. Allerdings funktioniert dies dann aber nicht mehr, wenn man mal
malloc() aufgerufen hat, weil dann das initialisierte Pattern am
"Ram-Ende" überschrieben wird.
http://www.mikrocontroller.net/topic/238822#2424929
/ /

• mem_check.c
• Created: 14.05.2014 12:56:50
  */

#include <avr/io.h> // RAMEND
#include "mem_check.h"

// Mask to init SRAM and check against
#define MASK 0xaa

// From linker script
extern unsigned char __heap_start;

unsigned int get_mem_free (void)
{
return SP - (uint16_t) &__heap_start;
}

unsigned short get_mem_unused (void)
{
unsigned short unused = 0;
unsigned char *p = &__heap_start;

do
{
	if (*p++ != MASK)
	break;

	unused++;
} while (p <= (unsigned char*) RAMEND);

return unused;

}

/* !!! never call this function !!! */
void attribute ((naked, section(".init3"))) init_mem (void);
void init_mem (void)
{
__asm volatile (
"ldi r30, lo8 (__heap_start)" "\n\t"
"ldi r31, hi8 (__heap_start)" "\n\t"
"ldi r24, %0" "\n\t"
"ldi r25, hi8 (%1)" "\n"
"0:" "\n\t"
"st Z+, r24" "\n\t"
"cpi r30, lo8 (%1)" "\n\t"
"cpc r31, r25" "\n\t"
"brlo 0b"
:
: "i" (MASK), "i" (RAMEND+1)
);
}`
There is one important remark, get_mem_unused() reports the free memory which is untouched. This does not work after once malloc() was called. And malloc() is called by "new CLASS" what is used.

I will have a look into your branches, thanks!

[GioCC]

Another option, which could be the most sensible in an embedded environment like the Arduino, would be to get completely rid of dynamic allocation (which is known to be something best avoided). Reserve a fixed buffer, large and fully controllable during compilation, and manage it explicitly; after all, all we need is to either incrementally allocate chunks of memory, or free up everything and start from scratch, therefore management would be almost trivial.
This would also allow to dispense with the allocation of a fixed max number of units for each device type, meaning the user could decide to use up the whole memory to allocate e.g. buttons rather than displays (obviously according to the limited number of pins available). This might become crucial with the introduction of input shifters, multiplexers and I/O expanders.
Also, getting rid of the config buffer in RAM is something that's becoming overdue.

[neilenns]

@GioCC I was thinking of something similar after reading around online a bit. Was thinking of looking for a library that would handle the management, but as you point out the needs of the firmware are pretty simple.

[GioCC]

TBH it's something that I had in my bucket list, but (a) there's already very much going on at the moment (Hooray!!! :D) and (b) I won't claim to be able to work 25/8, even during the holidays... but I'm certainly up for it as soon as I manage to find the time.

[elral]

Very strangge things are going on. And now after a couple of hours I found out that MemInfo functions mentioned above from Neil can also cause a crash.
OK, so starting again from scratch with my investigations...

[elral]

Seems that I got now a stable version for the Uno. I have to test it for the ProMircro too, but unfortenutely I bricked him during my testing. I have to find some time during the weekend.

It was only possible to reduce the RAM by every idea I have in mind and we all habe discussed before. Espacielly reduncing the configBuffer size (only the names from the inputs are now stored) was a main driver, but also some other "small" modifications.
I need to clean up my branch with all my debug prints, comments, ... before I could make a pull request.
I do not look optimistic for merging the multiplexers as I except some more RAM usage for them and it is getting really really short.

Another option would be, to init all possible devices dynamically and issue an reset after uploading the config. Allocation of memory seems to working fine.

[GioCC]

@elral about the multiplexers (or any other device, really) you're right, in fact I had to temporarily disable Steppers in order to have at least enough memory to get them working; and the real offender BTW was the flash space (obviously RAM was on the edge too).

[elral]

@GioCC yes, the stepper lib needs quite a lot of RAM and Flash. Even with the RAM saving I have implemented and maybe same additional one (I expect only minor savings futhermore like Neil also stated in issue 136) it could be that more devices do not fit anymore into the RAM/Flash of the Uno/ProMicro.
And that could happen with your multiplexer, whereby it should be also discussed if it makes sense for the Uno/ProMicro (best under the discussion about this in the connector repo) and not limited to the multiplexer.

And by the way, I got again a worse finger from scrolling with the mouse through the "big" mobiflight.cpp file. ;)
I had a short discussion with @DocMoebiuz some days ago and @neilenns also proposed it already to split up the mobiflight.cpp into several files. I (and Neil?) have prepared a branch , but I guess it would be better to open a separate discussion/issue.

[GioCC]

Unfortunately I think we're already scraping the bottom of the barrel by just trying to optimize individual devices (which is still welcome anyway, where possible).
In my opinion the most promising way to go to reclaim memory is to get rid of the RAM config buffer (this is what you told me you were already working on, isn't it?) and most of all, controlled dynamic allocation of required devices. Having pre-allocated arrays for a fixed number of devices the you will not use is something that makes me itch...
For Flash space, modular compilation would be the ideal solution, but as already explored, this is a little more complicated feature. I do not lose hope that we'll probably end up addressing it in the long run.

About the length of the Mobiflight.cpp file... glad to hear this has been brought forward! I had the exactly same feeling, and I would have proposed that too sooner or later... Let's open a discussion, by all means, I'm interested.

[elral]

Regarding the configBuffer I fully agree, and I already prepared everthing. Parts of it I used for reducing the size as now only the names of the input devices are stored in the configBuffer. It is an easy next step to get rid of the complete configBuffer.
But, this would mean changes on the connector side I am not able to do this. And it is a topic of backwards compatibility what @MobiFlight-Admin mentioned to me. Means these changes result in a firmware update too. This was not required in the past and is a question of user acceptance. For now I have not build an opinion for myself.

Regarding the dynamic memory allocation I am also not sure how to proceed. Allocating does work, but not freeing the memory. A solution could be to issue a reset after config upload (not sure how to do this for the ProMicro), but then it has to be ensured that not too much memory gets allocated by too much devices. Some kind of calculation has to be done while adding each single device, either in the firmware (preferred) or in the connector. If this is not done the Arduino could crash during startup and even the ProMicro can get bricked.
For now I see a good chance to get it stable with the existing devices, but checking it for the ProMicro I have to shift until next week as I brocked yesterday my ProMicro mechanically (USB connector).
But it has to be decided how to proceed...

[GioCC]

Sorry, I realized maybe I created some confusion.

My aim is not to use dynamic allocation properly meant.
The proposed solution would be to reserve a "big" static buffer with a manager class that incrementally assigns chunks of it to whatever device requests it.

I called it "dynamic" as opposed to "fixed preallocated space for a fixed number of devices".

As I mentioned somewhere else, the manager would be almost trivial, since we don't have to face complex mechanisms (allocation is only incremental; deletion is global).

Correct object instantiation could be done using placement new; it would almost be the same as using "new", but without using it (and without the need to linking in the code for new/free memory management).

A few pointers if anyone should require them:
"Placement" syntax
Static allocation of objects

[GioCC]

Probably not the correct place for this, but I'll just also mention another point:

the main drawback of the "allocation manager", and currently my main worry about this, is that the GUI would not be able to determine when the user is running out of space and therefore can no longer allocate devices.

Currently, the limit warning is based on the known max device numbers; in order to work in the new situation, the GUI should be aware of the actual size in the firmware of the device objects it's requesting (and obviously the buffer size).
This is not impossible, but definitely not as straightforward (or at least, it's a little annoying and error-prone to find the values).

Another interesting option could be that the firmware returns the amount of remaining free space after each device configuration is sent; this would allow to show a "fuel gauge" that informs the user progressively during configuration.
This option would require the GUI to NOT send the whole configuration in one single message (and that would be easy to change); however, the firmware should then be able to remove and re-add devices one by one, which would make the allocation manager much less simple.

[elral]

Does somebody know what this warning means and how to avoid this?
src\MF_Modules\MFLCDDisplay.cpp: In member function 'void MFLCDDisplay::detach()': src\MF_Modules\MFLCDDisplay.cpp:40:10: warning: deleting object of polymorphic class type 'LiquidCrystal_I2C' which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor] delete _lcdDisplay;

What I have read this "will" cause problems and not "might" cause problems, but i am not sure.
Declaring the LCD statically will not give the warning but then the memory is not enough.

I am also wondering if deleting all classes must be in the opposite order as they are initialized to avoid fragmentation of the memory...

[GioCC]

I believe that boils down to none of the classes in the derivation chain (Print <- LiquidCrystal_I2C) having a destructor defined as virtual, therefore delete() can't guarantee that all destructor(s) according to the class of the actual object will be executed.
As a matter of fact, defining a virtual destructor in either of the two classes (e.g. virtual ~LiquidCrystal_I2C(){}) satisfies the compiler; not sure why, because I would say that "Print" is the one required to have it (~LiquidCrystal_I2C() should be aware that it has to call ~Print()).
However, I think that as long as you're sure that you only delete objects of the derived class, everything is fine.

[GioCC]

About the order of deletes, that should not also matter, as long as you delete everything in the end (as we do); and then, I would expect everything to be in order, unless the heap management really sucks (which might well be, in a small embedded system).

But don't just take my word, I haven't factually verified this.

[elral]

@GioCC

As I mentioned somewhere else, the manager would be almost trivial, since we don't have to face complex mechanisms (allocation is only incremental; deletion is global).

Correct object instantiation could be done using placement new; it would almost be the same as using "new", but without using it (and without the need to linking in the code for new/free memory management).

from a high point of view I understood what you are proposing, but it seems that some deeper knowledge is required. I had a look around regarding placement new, but only less information espacially regarding classes can be found. The less examples I already tried to transfer, but I got compilation errors while using this (even I read that Arduino should support this since 2019). The other topic for me is how to calculate the required space in the memory for one class, some more reading seems to be required. But if this is running it seems to me the perfect solution to use the available memory as good as possible.

I am still unsure how to proceed with this issue. Still "waiting" until this concept is running or to implement an interims solution where all classes are defined statically and to reduce the memory as much as possible.
My opinion is to go with the interims solution and later on to implement the memor handling and go back to init the classes statically.

[elral]

Ohmm, #include does help ;)
And for one class it seems to work, now calculating the next free memory loaction needs to be done...

/edit 2: And now it works with 2 LCD's :) Checking if still memory is available needs to be implemented but seems that it could be done!

[GioCC]

@elral Not sure to what part(s) you are referring to exactly, but I have the feeling it's something very promising! :D

[elral]

@GioCC , I took your execellent proposol regarding placement new and wrote a small function to allocate the memory for the output devices within a static buffer. I only forget to #include <new> which cost me a lot of hours :(
Clearing the memory is much easier as you already told, we don't have to care about deleting single devices.

Next I declared LCD, 7Segments, Servo and Stepper dynamically while using this new static buffer. And what should I say, the first tests are showing that everything is working fine. I can upload mulitiple times a config on the Uno without crashing him. Unfortenutely I have no devices connected, but I made the same test on the Mega where I have most devices connected. And all are working. The part what is missing is, that before initialisation of a device it has to be checked if sufficient memory in the static buffer is available. But this should be now an easy task.

allocateMem.cpp:
#include <Arduino.h>
#include "MFBoards.h"
#include "mobiflight.h"
#include "allocateMem.h"
char deviceMemory[MF_MAX_DEVICEMEM] = {0};
uint16_t nextPointer = 0;
char * allocateMemory(uint8_t size)
{
uint16_t actualPointer = nextPointer;
nextPointer = actualPointer + size;
if (nextPointer >= MF_MAX_DEVICEMEM) {
cmdMessenger.sendCmdStart(kDebug);
cmdMessenger.sendCmdArg("BufferOverflow!");
cmdMessenger.sendCmdEnd();
return nullptr;
}
cmdMessenger.sendCmdStart(kDebug);
cmdMessenger.sendCmdArg("BufferUsage");
cmdMessenger.sendCmdArg(nextPointer);
cmdMessenger.sendCmdEnd();
return &deviceMemory[actualPointer];
}
void clearMemory() {
nextPointer = 0;
}

allocateMem.h:
#ifndef allocateMEM_h
#define allocateMEM_h
#include
extern char deviceMemory[];
char * allocateMemory(uint8_t size);
void clearMemory();
#endif
Within e.g. MFStepper.cpp
#include
_stepper = new (allocateMemory(sizeof(AccelStepper))) AccelStepper;
instead of
_stepper = new AccelStepper();
and NO
delete _stepper;

Same for the other devices which are initialized dynamically.
clearMemory(); is done in resetConfig().

I am pretty sure that some optimazation and tests are required, but in principle it seems to work.

[GioCC]

That's great! As I mentioned, it looks pretty straightforward (which means much less chance of bugs or issues), and yet very effective. I'm looking forward to the significant improvements that can be achieved! Bravo Ralf ;)

[elral]

Thanks!
If this really works stable, we can think about to init more devices dynamically (where it would make sense). On the bad side this would mean changes on some devices. But the advantage would be that the user has much more flexibility in choosig these devices he need. He is "only" limited by the maximum of the buffer.

[DocMoebiuz]

Sounds super cool! Congrats guys for making these first steps - i was really worried in the beginning when i first read about it. That would be very very cool

[GioCC]

Exactly! And I think it can be rather groundbreaking. Think e.g. of getting rid of LCD, Servos and Steppers in one go for a board that does not use them. How much more space you can gain for other stuff (or the opposite, of course)!

[GioCC]

@DocMoebiuz You can trust us... we mean no harm ;)

[neilenns]

This is fascinating, I had no idea "placement new" was a thing!

[DocMoebiuz]

I meant i was worried it might become clunky and cumbersome but it looks pretty straight forward and not overly complicated.

[DocMoebiuz]

@DocMoebiuz You can trust us... we mean no harm ;)

now that you say it explicitly, i am starting to get worried 😂

[GioCC]

Sorry I meant...: Resistance is futile. 😂😂😂

[elral]

Some more tests shows clearly, that we have to save RAM where ever we can for the Uno and ProMicro. For the Uno I ended up with a configBuffer of 50Bytes (only Input names will be saved) and a "DeviceBuffer" of 120 Bytes to get the Uno stable (multiple config loads w/o crash). Compiling with these sizes results in 1.898 Bytes from 2048 Bytes. If the buffersize will be increased, the first startup still works. But a config upload results in a crash, I think the heap and stack gets overlayed. Looking how a board reset is done, a couple of functions call other functions. And in the end "only" 150 Bytes are left.
For the ProMicro with 500 Byte more RAM the situation is slightly better. Maybe l will save a couple of bytes after cleaning up the code and deleting all the debug prints :)
The memory consumption of the "DeviceBuffer" from the devices is:

• Stepper: 68 Byte
• 7Segment: 24 Byte
• LCD: 14 Byte
• Servo 3 Byte

The next step will be to setup a new clean branch with all the changes and some more testing.

[elral]

I did some more changes, all devices are now initialized dynamically. This will save some more memory which can be used for the new "deviceBuffer" and the ConfigBuffer. This also adapted now for the ProMicro and Uno.
The memory consumption for the device is now:

• Encoder: 31 Byte
• Button: 4 Byte
• 7Segment: 24 Byte
• Stepper: 68 Byte
• Servo: 3 Byte
• LCD: 14 Byte
• Analog: 29 Byte
• Output: 2 Byte
• Output Shift Register: 9 Byte
• Input Shift Register: 11 Byte

The Uno runs stable with a deviceBuffer of 250Byte and for the ProMicro is a deviceBuffer of 500 Byte defined, but for now untested. These buffersizes can be used for the device (size see above) until the buffer is full.
Multiple config uploads does not lead to a crash with different configs I have tested.

But it would be really nice if somebodey could test it too. In this case i can provide the link to my branch. The branch is not cleaned up and needs some more checks, but runs stable. Debug Info's are transferred to the Connector and will show up in the lower terminal window (I can also provide this branch).

[DocMoebiuz]

that's great! have you done the math to check whether the currently defined number of devices for the boards would fit into these buffers?

[GioCC]

I can't promise about how much testing I will be able to do these days, but I would appreciate if you could send me the link to the code to have a go.

[elral]

Oh je,
the firmware can be found under https://github.com/elral/MobiFlight-FirmwareSource/tree/AccelStepper_static, the Connector with very less changes in MobiFlightModule.cs (DebugPrint) under https://github.com/elral/MobiFlight-Connector/tree/debug_print

All devices (incl. InputShifter) together need (size of deviceBuffer):
Uno: 462 Byte (286 Byte)
ProMicro 713 Byte (440 Byte)
Mega: 2350 Byte (2000 Byte)

The branch is not cleared up and has a lot of uncommented debug prints and a lot of changes to reduce memory size which I guess will need some explanations. But I have less time in the moment as I will start my vacation today.

[GioCC]

Thanks, I'm looking forward to having a good look at it. In the meantime, enjoy your holidays!

[GioCC]

A random thought: you could save some more bytes by using the F() macro in the debug prints (cmdMessenger.sendCmdArg("...")), at least for the arduinos which are not already almost out of flash.

[elral]

This issue might be related to issue #140.
Some more tests are required.

[elral]

While working on this issue and creating the pull request, I also touched the part of uploading the config (already changed to write directly to the EEPROM).
But there is still one topic which I don't really understand and I can't see by evaluating the firmware.
The order of uploading a new config for the first steps is clear:

1. with kResetConfig (13) all devices are cleared
2. with kSetConfig (11) the config is uploaded in multiple blocks
3. with kSaveConfig (14) the config is stored into the EEPROM (but now changed, not required anymore as the config is directly written to the EEPROM)

The last step I don't really understand. There are two functions available, kActivateConfig (16) and kResetBoard (24). With additional debug prints to the connector it seems, that both functions are called, but I can't figure out in which order (debug prints are sometimes mixed in the timeline and this happens here). I wonder if really both functions are send from the UI and if so, what is the reason? From what I can see in the firmware it should be enough to issue an kActivateConfig (16) which calls the function OnActivateConfig(), kResetBoard (24) is for my understanding not required.

Can someone point me to the file from the UI where the config upload is programed? Or does someone know which functions are called?

[GioCC]

If I haven't misread what you need, the config upload is made in MobiFlightModule.cs / SaveConfig() (around line 770).
Looks like it's the only place where the commands Command.SetConfig / Command.ResetConfig are issued.

SaveConfig() is invoked from MobiFlightPanel.cs / uploadToolStripButton_Click() (around line 800); once finished, MobiFlightModule.cs / ResetConfig() is then invoked (which again is the only place where Command.ResetBoard is issued).

[neilenns]

When I was working on my custom firmware that fakes out MobiFlight and generates its config on the fly here's the flow I traced when saving a config from the desktop:

1. kSetName
2. kResetConfig
3. kSetConfig
4. kSaveConfig
5. kActivateConfig
6. kResetBoard
7. kGetConfig

[elral]

@GioCC yes, that's what I am looking for.

@neilenns oh yes, good hint with kSetName and that a kResetBoard is really initiated after kActivateConfig.

But then I am still wondering why the kResetBoard is done. After kActivateConfig everything is done, a ResetBoard seems not to be required....

[GioCC]

@elral Really looks like a leftover from a previously used (or different) sequence... in this procedure it actually seems redundant, I imagine as a command (i.e. besides obviously on boot) it could make sense if the config buffer on RAM was somehow thought to be corrupted - but even in that case, not by an upload error, otherwise the EEPROM would have been likely corrupted too.