-
Notifications
You must be signed in to change notification settings - Fork 17.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AP_Scripting: allow expansion of memory for scripting at runtime #26517
base: master
Are you sure you want to change the base?
Conversation
f942493
to
f91f37c
Compare
My only real concern is that the user won't notice. I think we should call the error handler in scripting so it will make a fuss and print the warning lots and will trigger a pre-arm. If we do that I think its fine to allow it to run when disarmed too. Currently just running out of memory only tells you that there is not enough, I think this method would be able to tell how much is needed. Ideally the users gets this error then puts the value into the heap size param and then never sees it again. |
that would make their script fail, which loses the main reason for this PR which is to handle cases where a less common path in a script (eg. a failsafe handling) takes you over your memory limit and a critical script fails |
Sorry, bad wording, I didn't mean you actually error out, just call the |
When this says "makes it possible", what does that mean? From the dev call discussion it sounded like the RAM will be automatically allocated if needed when armed, which actually makes sense, but that's a bit more proactive than "makes it possible". Could you make it a bit clearer what's actually happening? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes to make things cleaner.
f91f37c
to
98c2963
Compare
020e939
to
fc769f8
Compare
I've now added that in, message like this:
|
11f2a13
to
265df41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested only by inspection, but this looks like a good set of changes to correct the problem for 4.6. I don't think it's worth digging into the Lua interpreter for now but fixing the HAL now will let us clean up and iterate during the 4.7 dev cycle while still fixing the problem for 4.6.
Also don't forget the test suite!
b7da78c
to
12783e4
Compare
Tested the aerobatics script on SITL and SimOnHW and this does seem to work as advertised. Unfortunately bumping the heap size up to what the warning reports is not a guarantee that you won't get a bigger warning next time presumably due to allocation patterns but we aren't running into flight controller memory issues yet so it should be good for now until scripts get even more complex. I think the warning user experience is decent, though having it repeat so many times in flight for a less critical thing is annoying. Maybe it should be downgraded to info? The point is that you don't have to stop the flight, but it will automatically become a prearm fail and an error upon landing no matter the in-flight severity IIUC. I don't take any position on if this deserves a backport, but I think this is a good set of changes for doing such if desired. Simplifying/reducing it more will make backporting awful. |
This makes things worse on ESP32S3, I can't get scripting to allocate at all though I'm not really sure why and it (barely) works on master... It's not a huge deal but disappointing. Have you given it a try on QURT? |
make functions for lua heap allocation suitable for use in all non-ChibiOS HALs
this allows for new heaps to be added at runtime for lua scripting if you run out of memory while armed
we want the lua garbage collector to be used to re-use memory where possible. This implements a suggestion from Thomas to avoid heap expansion unless the last allocation failed
implement in AP_MultiHeap instead
this is a standalone (no-HAL based) implementation of MultiHeap
993ba35
to
a17f44b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, ESP32 can be fixed later.
Still need buy-in on the backport.
This makes it possible to expand the MultiHeap used by scripting at runtime. This means if the user underestimates the right value for SCR_HEAP_SIZE and they have free memory (which they usually do on a H7) then it will allocate more memory with a warning
Note that:
This PR also cleans up the handling of scripting debug options
The motivation for this PR is there is really no way to predict in advance how much memory a set of scripts will need, so users may under-estimate and the scripts fail at a critical time
This PR also fixes an issue where realloc may fail if there is no room in the current sub-heap. We now always alloc/copy/ree. ChibiOS was doing this within a heap already, so this doesn't cost any more, but means it fixes a path for script failure
This was test flown on a Pixhawk6X
update: moved the allocation code to a separate AP_MultiHeap library, removed the hal.util functions. Tested in sim-on-hw and SITL