Skip to content

Add time capsule to store strings between supervisor reloads #4597

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

Closed
wants to merge 5 commits into from
Closed

Add time capsule to store strings between supervisor reloads #4597

wants to merge 5 commits into from

Conversation

TG-Techie
Copy link

@TG-Techie TG-Techie commented Apr 13, 2021

First off, this Pr is not done and will need to change before it is ready.

Since this is my first significant change to the core, I'm not fully aware of what needs to change; I thought a pr would be the best forum to discuss modifications.

Goal

Add a module to the circuitpython core that allows a limited length string(1024 for now) to survive between supervisor reloads.

The proposed name for this is capsuleio. It can store some data in a "time capsule" that can be dug up between supervisor reloads.

Uses

The most useful application of this will be user code using it to select what file to run between reloads without writing to the file system (which can dangerous) or nvm (which may eventually wear out).

capsuleio could be used to dispatch between apps on bootup:

# code.py

import capsuleio
import supervisor

message = capsuleio.unearth()

if message is None:
    import app_switcher
elif message.startswith('app:'):
    # assumes apps are python modules in stored in /apps
    __import__(f"apps.{message[4:]}")
else:
    # either
    raise RunTimeError(f"unknown casupleio message {repr(message)}")
    # or
    capsuleio.bury(None)
    supervisor.reload()

I think this leaves less room for accidents and allows more flexible usage.
This code doesn't need to be in code.py, and it can store more than "what to run next".

For another example, I'm working on a smartwatch that runs some system code (for brightness slider, settings, etc). In this context, only changing what file is run when the system reloads would not work as the watch needs to load several libraries and init the above-mentioned system code.

There are all sorts of tricks that could be used to stuff data into the time capsule; if 1023 is not enough, data could be hashed to fit, or a bytes object could compactly store numbers.

Current implementation

Right now, the module reserves a global struct that can store up to 1024 bytes and a tag to indicate what kind of data it is storing. (called capsuleio_capsule)

This implementation does assume that globals are initialized to zero. (iirc this is in K&R 4.9 and C89?)

When capsuleio.bury(obj) is called, it checks if obj is a string or None. If not, it raises a TypeError. If it is a string and the length is sufficiently short, it memcpys the string data into capsuleio_capsule. This way, only the value is ever stored, never a reference.

I think there is wiggle room for more clever implementations. However, I don't know if it is necessary, and this only stores the string/buried data, a byte for the type kind, and a null terminator (just in case).

What I think needs work

  • separation of shared-binding and shared-modules.
  • duplication of reading data
  • In-code python docs: Completely did not add them, I will.
  • bringing it to all the ports (just yet to do)
  • non-string or None data storage?

Separation of shared-binding and shared-modules

Right now, the business logic is spread between and mixed across shared-binding and shared-modules. I'm not quite sure what should go where exactly.

In short, should the whole implantation be in shared-binding?

In a brief discussion with @dhalbert he mentioned that shared-modules are sometimes called outside of a VM context. I don't think this ever would be needed; if so, I could move it all into shared bindings.

Duplication of reading data

Right now, when returning a string form .unearth(), a new string is allocated every time. Do we want to support a use case where capsuleio.unearth() would be called more than once?
I choose to make a new string every time to avoid memory leaks and eliminate the need for any initing code on supervisor start-ups

Possible future features

It would be interesting to allow storing ints, float, and tuples. However, these can all be serialized into strings or bytes objects.

suggestions and discussion more than welcome!



// the time capsule memory reservation
capsuleio_capsule_t capsuleio_capsule;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think think this is a reasonable thing to do. Have you considered such boards as the QT PY? This will take 1/32 of its total memory. Probably many folks are running those right on the edge and 1kb would push them over the edge and unable to update.

What does the data sheet say on typical circuitpython boards w.r.t. program/erase cycles on the embedded flash, and what’s the typical cycle count on express board flash? I imagine if you take into account whatever load leveling is there you’ll find that it’s expected to live decades or more with a 1kb text file written occasionally. (I.e., is this solving a real problem?)

Maybe instead you want to learn more about the garbage collector & memory allocator and mark a capsuled tree as uncollectible? In this way the memory cost would be dynamic and opt-in rather than a high-water mark for the largest allocation somebody changes the capsuleio_amount_bytes const to.

Copy link
Collaborator

@dhalbert dhalbert Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alarm.sleep_memory and microcontroller.nvm both provide persistent storage with a simple bytearray-style of storage. Rather than pass a string, arbitrary bytes is more flexible, though a bit less convenient. Anything can be converted to bytes. The API of those modules would be what to follow. This would be called supervisor.persistent_memory or something like that.

@WarriorOfWire I have seen use cases of needing persistent storage and restarting something every few seconds. For some chips I calculated this would reach the specified flash lifetime after a few weeks. (Useful rule of thumb: a million seconds is about 11 and a half days.)

Rather than reserving a chunk of RAM permanently, the data in question can be copied out of the heap to a safe place when the VM exits, and then put back after the restart. The "safe place" might be on the stack, or just someplace that's untouched while the VM is not running. It might even work to leave it in its old heap location. As soon as the heap is re-created, a new heap object of the right size can be created and the old data copied there.

Copy link
Author

@TG-Techie TG-Techie Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API

alarm.sleep_memory and microcontroller.nvm both provide persistent storage with a simple bytearray-style of storage

@dhalbert That sounds like a much better interface, especially since that mirrors the literal, physical underlying hardware. I really like your suggestion of putting it with the supervisor namespace as the "time capsule"* should share the same lifetime.


@WarriorOfWire: Agreed 1k is not a reasonable amount of memory for non-full builds of cp. The ..._CAPSULE_AMOUNT_BYTES should definitely change for the size of the build (maybe m0 = 128, m4/nrf=512?, imx=even more?).
It's also not uncommon for 'niceties' like gamepad, json, or ulab to be excluded from small builds, maybe only full builds of cp should include this?

Implementation

Maybe instead you want to learn more about the garbage collector & memory allocator and mark a capsuled tree as uncollectible?

Opt-in does sound a lot better than consistent memory hit and at the same time I'd be wary to mark an arbitrary tree of python objects as 'do not collect'? Though the supervisor.time_capsule* interface would not have that option.

Rather than reserving a chunk of RAM permanently, the data in question can be copied out of the heap to a safe place when the VM exits, and then put back after the restart.

Could you elaborate, please? What is the advantage of copying in and out of the safe place vs storing directly to the safe place?

separate thought

A side note on naming, 'persistent' can be another name for 'non-volatile' in the memory hierarchy. IIRC previous conversations have discussed calling it "scratchpad" but that may not be clear.

An additional difference is on disk and .nvm will persist between resets, not just reloads.
The guarantee that the "time capsule"* will always init to all zeros on reset. I think there is some safety there, though that was not a motivating reason to implement this.

* whatever the final name becomes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate, please? What is the advantage of copying in and out of the safe place vs storing directly to the safe place?

It doesn't have to be pre-allocated and use up precious space in RAM if not used.

I have a somewhat better API idea:

Don't provide a fixed-size area but to instead just do something like supervisor.persisted_bytes = some bytearray or bytes. So it could be as large or small as you want. When the VM exits, the bytes are either left where they are (as described above) or copied to someplace temporary (like the stack). When the VM restarts, a new object with the bytes is created, you can fetch it via supervisor.persisted_bytes. If you want to throw away what you persisted, you can do supervisor.persisted_bytes = None.

Copy link
Author

@TG-Techie TG-Techie Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @Neradoc pointed alarm.sleep_memory has the same functionality.
The main differences appear to be:

  1. It is port dependent and seems to only be supported on esp32s2 (stably that is)
  2. It is grouped with the alarm module meaning implementing it is paired to the alarm module
  3. It is fixed length where this could be dynamic length

Possible Solution

merge alarm.sleep_memory and capsule as a supervisor.saved_memory* singleton with a max length that is included in only full builds of python.

For boards that do not have specialized features, a common implementation could do the on-stack (or safe place or pystack) swap. Here having a max length means you can't request to save something so large it would overflow the stack.

For ports that have specialized peripherals/memories, it is implemented on top of those.

The max length would vary per port.

*still working title

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why limit the max length?
  • Why take ram away from devices whether they use the feature or not?
  • If you prefer, the previous 2 can be summed up as “why does this use static allocation?”
  • supervisor.persisted_bytes was proposed as a name to use. I +1 that name.

@WarriorOfWire: Agreed 1k is not a reasonable amount of memory for non-full builds of cp.

This is not what I attempted to communicate - I intended to represent that statically allocating a buffer that will be rarely used and is not even in a hot path is hostile to users’ memory. It is hostile whether it is a full build or not. 128 bits is 10 vectorio circles with xy locations. It’s not insignificant on any build and this memory cost ought only affect users of the api.

Choosing different buffer sizes per board also makes it more painful to use - and to Dan’s point which I share, it’s needless right?

Copy link
Author

@TG-Techie TG-Techie Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for re-framing your point and elaborating on Dan's. I now see how I miss interpreted both of your statements and guidance.
I hear you regarding statically allocating and how having the persisted data be any size tailors any cost involved not to the board or build but rather to the specific use case/call (?). That went right over my head for a bit: thank you for explaining.

I have a few questions to make sure I head in the right direction:

  • Would it make sense for the message saving (whether by marking uncollectable or copying) would go somewhere in or between stop_mp, free_memory, or supervisor_move_memory? link
  • If so, where would you suggest putting the message restore procedure call? I'd hazard a guess it would go somewhere in or between stack_resize, allocate_remaining_memory, and start_mp? link

Copy link
Author

@TG-Techie TG-Techie Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the right direction" being towards supervisor. persisted_bytes as Dan has suggested above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would store .persistent_bytes in the supervisor module dictionary, which we would make mutable. We would treat it similarly to the .wake_alarm attribute in alarm, whose module dictionary is also mutable. gc_collect_ptr() is called specially on the .wake_alarm object, for instance.

{ MP_ROM_QSTR(MP_QSTR_wake_alarm), mp_const_none },

I do not remember if anything is done with the heap when the VM is stopped. That would have to be researched. If the heap is left alone, we don't need to copy the bytes until we start up the VM again, before we reinitialize the heap. The bytes could be copied onto the stack, the heap would be initialized, and then a bytes object would be allocated for the saved bytes and they would be copied in. Since the original object might be anywhere in heap storage, I don't think we can rely on it not being smashed up in some way when the heap is initialized (but I don't know the details of heap initialization either).

This is somewhat complicated and will require tracing through some internals stuff to make sure. I cannot say for sure "put some new code here and there" without re-understanding the detail VM and heap shutdown and startup code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think others are better informed about the lifecycle of the Circuitpython init/teardown than me, though I do need an excuse to use this Segger. I’d love to see what Dan has to say there ❤️
Also to be clear, I don’t particularly care whether it’s a copy or marked objects. My care is that users should only “pay for what they use” as far as ram goes and just about any dynamic allocation strategy will accomplish that.

@Neradoc
Copy link

Neradoc commented Apr 13, 2021

Would it be reasonable to merge that with the alarm sleep memory ?

  • implement this the way alarm.sleep_memory is implemented where alarm is available.
  • remove alarm.sleep_memory, and use this instead.
    The use case is similar: remember data when exiting and reentering code.py, without using the flash, reset on power cycle / hard reset.

@dhalbert
Copy link
Collaborator

Would it be reasonable to merge that with the alarm sleep memory ?

  • implement this the way alarm.sleep_memory is implemented where alarm is available.
  • remove alarm.sleep_memory, and use this instead.
    The use case is similar: remember data when exiting and reentering code.py, without using the flash, reset on power cycle / hard reset.

The use case is similar, but the implementation may not be. In the deep sleep case, we go almost all the way to a reset. On some chips, we can turn off some but not all of the RAM, and store sleep memory there. On others we can leave RAM on. On yet other chips, we have to use special registers or a special non-regular-RAM area that is maintained while the rest of the chip sleeps. It varies wildly.

@tannewt
Copy link
Member

tannewt commented Apr 13, 2021

I agree that the sleep memory is a better option. Also, this PR for selecting the next file to run is pretty close: #3454 I'd much rather see that finished than add another way to store arbitrary data across runs. The more state that is unique to a particular run, the more difficult debugging will become.

@dhalbert
Copy link
Collaborator

Yet another way of passing data to a program that has started would be via sys.argv, which is non-CircuitPython-specific.

@TG-Techie
Copy link
Author

TG-Techie commented Apr 13, 2021

I agree that the sleep memory is a better option

Does all of alarm need to be implemented at once? can just alarm.sleep_memory be implemented depending on the port?

@tannewt
Copy link
Member

tannewt commented Apr 13, 2021

Yet another way of passing data to a program that has started would be via sys.argv, which is non-CircuitPython-specific.

That does remind me! I would like to have os.environ it's very often used for passwords and other secrets. #4212

Does all of alarm need to be implemented at once? can just alarm.sleep_memory be implemented depending on the port?

No. The expectation is that individual alarm types may or may not be available on each platform.

@TG-Techie
Copy link
Author

TG-Techie commented Apr 13, 2021

That does remind me! I would like to have os.environ it's very often used for passwords and other secrets. #4212

I presume os.environ would not persist between reloads 😉?

I am more than willing to contribute (to the best of my ability) a "data past reload" functionality if it would be helpful.
Both persisted_bytes or sleep_memory sound good to me though keeping to one API sounds much better.
It looks like sleep_memory fits the same use cases as this so it doesn't sound like this is necessary.

[Edited]
It sounds like my efforts may be better directed to other parts of CircuitPython?
Unless this is a development path we want to follow, I'll keep looking for other ways to help!

as an aside

it looks like progress on adding sleep_memory to the nrf branch is already well underway in #4236 (which, thank you dan. looks like you're one of the contributors )

@deshipu
Copy link

deshipu commented Apr 14, 2021

Is this related to the work that has been done in #3695? Should the same mechanism be used?

@deshipu
Copy link

deshipu commented Apr 14, 2021

The #1084 also seems relevant.

@tannewt
Copy link
Member

tannewt commented Apr 14, 2021

Thanks @TG-Techie. My recommendation would be for you to look at #3454 . I think it's primarily what you want and would also be very nice to get merged in.

@dhalbert dhalbert marked this pull request as draft April 23, 2021 13:35
@TG-Techie TG-Techie closed this May 17, 2021
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 this pull request may close these issues.

6 participants