Skip to content
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

Lack of Error Signaling when insufficient app memory available #1764

Closed
hudson-ayers opened this issue Apr 16, 2020 · 5 comments · Fixed by #1770
Closed

Lack of Error Signaling when insufficient app memory available #1764

hudson-ayers opened this issue Apr 16, 2020 · 5 comments · Fixed by #1770

Comments

@hudson-ayers
Copy link
Contributor

By default, libtock-c applications request 8kB of memory. On a board like Hail, NUM_PROCS=20, implying that 20 processes can be loaded. However APP_MEMORY on hail is set to 49 kB, so attempting to load more than 6 processes fails. Currently, this manifests as the first 6 processes loaded onto the board work as expected, and subsequently flashed apps are treated as if they do not exist. No error messages are printed and the only signal something is wrong is that some apps do not execute.

A developer encountering this problem can solve it by increasing APP_MEMORY, or by decreasing the amount of memory requested by apps in libtock-c -- but it can be confusing to debug.

Ideally, when a board is initialized, it should at least print a debug message if there is not enough memory to load all apps, or perhaps even panic. Alternatively, perhaps tockloader could print an error if an app is loaded that there is not enough memory for?

@alistair23
Copy link
Contributor

If you enable the process loading debug does that give you enough information to figure out what is going on? Maybe that should be enabled by default?

@hudson-ayers
Copy link
Contributor Author

If you enable the process loading debug does that give you enough information to figure out what is going on?

Ah, yes it does! it says '[!] flash=[0x00041000:0x00041400] process=Some("t5") - couldn't allocate memory region of size >= 0xFC8'

Maybe that should be enabled by default?

I don't think we want to print all the process loading information in the default case -- I think the default should be to only print anything if there is an error

@hudson-ayers
Copy link
Contributor Author

Which would be a pretty easy fix (take the error reporting debug statements outside of the if config::CONFIG.debug_load_processes block) but I wonder if others will object to that? Perhaps the error should also be updated to suggest reducing the amount of memory used by the app? Perhaps error-reporting debug statements that happen only at initialization should be moved behind a different config flag that is enabled by default?

@bradjc
Copy link
Contributor

bradjc commented Apr 16, 2020

The line that is relevant here:

return Ok((None, 0));

Pre-TBF parsing changes addressing this issue would have been tricky, but now we can propagate an error to main.rs which will can then handle it as desired (right now we would print a message).

I think I didn't want to change too much in the TBF refactor, but looking at what is and is not an error when discovering and creating apps is something worth looking into. For example, should load_processes keep trying apps after not being able to load one? I think propagating an error in that case would be more difficult.


Which would be a pretty easy fix (take the error reporting debug statements outside of the if config::CONFIG.debug_load_processes block) but I wonder if others will object to that?

I would, but I think we already support a better solution using Result.

@hudson-ayers
Copy link
Contributor Author

Ah, I was working off a slightly outdated version of master that pre-dated the TBF parsing changes. Propagating that error to main.rs seems like the right solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants