InfiniTime icon indicating copy to clipboard operation
InfiniTime copied to clipboard

WIP: Generic memory persistence class

Open minacode opened this issue 1 year ago • 4 comments

This PR aims to implement a simple interface to store and load program state to the memory.

Why is this a good idea?

Because it provides a simple interface for developers and reduces the amount of IO-code people have to write. Writing persistent apps would only be a matter of defining the persistent data struct, inheriting from Persistent and loading/saving the data in the constructor/desconstructor (and maybe we can automate this away, not sure though).

What is already there?

In utility/Persistent.h is a new template class Persistent<T>. It holds a versioned struct of data which it can save to and load from the memory. The code is taken from the settings implementation. For testing/reference I changed the score of the Paddle game to be persistent.

What is missing?

Currently, I have a linker error I cannot get rid of. It's probably easy for someone who knows more about C++ than me. Therefore it's not tested either.

minacode avatar Nov 24 '24 17:11 minacode

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

github-actions[bot] avatar Nov 24 '24 17:11 github-actions[bot]

Love this concept! I'll see if I can look at the linker error sometime soon, this week is looking busy for me though :(

mark9064 avatar Nov 24 '24 23:11 mark9064

Hey there, I was asking similar questions (#2321), and was pointed here. I think I found the build error:

Line 32 of Persistent.h attempts to invoke a virtual method in the constructor of the template class. It looks like you can't do that there. I'm not a C++ expert, so I can't say for sure, but commenting out that line allowed the code to build (and the load method is called during the subclass instantiation, so it shouldn't make any functional difference).

I don't know if I can submit a change to this PR (I'm not a git/hub expert either), so hopefully this helps.

A couple other things that I found while looking at this:

  1. If a class defines GetPersistencePath() to return a path with a subdirectory that doesn't exist, the load and save functions don't work. I'm guessing you'd need to explicitly create any subdirectories first.
  2. You might want to change the return type of LoadPersistentState() to bool to indicate whether the file was found and was valid. That way the subclass can use default values or take some other action (technically you could do this by assigning default values before calling LoadPersistentState() but that seems less clear). While you're at it, it might be worth making SavePersistentState() return a boolean as well.
  3. The function definitions should probably be in a separate C file rather than in the header.

Another thing to consider: does this make more sense as a class template, or should the persistent data itself be the templated class? I ask because I can imagine a situation where an app might need access to more than one set of persistent data, or conceivably two apps might want to reference the same persisted data.

owenfromcanada avatar Jun 25 '25 17:06 owenfromcanada

Thank you, @owenfromcanada! That's a lot of good feedback 🙂

I will think about it and come back with a cleaner solution.

minacode avatar Jun 29 '25 14:06 minacode