moonboot icon indicating copy to clipboard operation
moonboot copied to clipboard

WIP: Add swap mode using InternalMemory/Flash scratch pages

Open Wassasin opened this issue 3 years ago • 5 comments

A proposal for using the internal memory to store the page that is being swapped, spread over multiple pages to maximize flash life expectancy. Also adds the swap exchange step to the current state, making the process robust against brownouts.

Possible improvements

  • Unsure if we should keep the older RAM-only swap mode.
  • Distinguish various types of Steps in state (as it is not needed for RAM swap)
  • Remove reference to concrete banks in state, and instead use an enum or index to reduce state size.
  • Use usize instead of u32 as many HAL and memory operations are based on usize and currently rather many conversions are required.

TODO's

  • Example in how to use this mode.
  • Perhaps implement some tests on a simulated processor.

I also implemented flash state on multiple pages for STM32 which is based on a linked list in order to maximize flash life expectancy. As it is rather specific to the type of flash and interface I have not (yet) added it to the repo. Perhaps we can discuss how to introduce it.

Wassasin avatar Jul 03 '22 18:07 Wassasin

Thanks a lot for implementing this, very nice!

One thought: With this library, I tried to keep as much as possible of the configuration in the types generics etc, to make it safe during runtime.

The Exchange scratchpad is defined via an array of Page indices (or a memory range?) in the Memory Banks. This can lead to a situation in which the firmware image written into the Update Bank is so large that it is written into the exchange pages. To keep this as generic as possible (like for example, scratchpad is in a different memory than the Update-/FirmwareBank, e.g. external EEPROM), a third Storage passed in there as a Scratchpad memory would make the distinction clearer.

This makes the usage more verbose though, as another Storage-instance with a small region has to be created (and the size should be a multiple of INTERNAL_PAGE_SIZE, even more complicated!). Additionally, the linker scripts which are generated need to know of a possible scratchpad region to reserve that space.

So to summarise: What do you think about treating the Scratchpad as a separate Bank? This way, it can either be a RAM-scratchpad or an Flash (Internal) scratchpad, currently determined via the MemoryUnit type. Unfortunately not decided at compiletime, but IMO this is OK

But this is of course open to discussion.

About the u32 Addresses: Yes, I also would prefer to use usize, this was just inspired by the embedded-storage crate, which if I remember correctly also uses u32 as an address type. But yeah, it should be changed to usize.

jhbruhn avatar Jul 04 '22 08:07 jhbruhn

About the flash state: That is also a nice addition to it, wear obviously might be a problem, especially when every exchange-step is stored in flash state. This is exactly why I made the State storage generic, so I'd be glad to integrate such an implementation into this project. It of course is required to make moonboot power-outage-safe, in addition to this PR! :)

jhbruhn avatar Jul 04 '22 08:07 jhbruhn

I have been thinking about the MemoryUnit-situation. We can either:

  • Abolish the use of the embedded_storage traits, and fork them to also take MemoryUnit into consideration.
  • Remove MemoryUnit and let the implementer of the embedded_storage trait do the heavy lifting by mapping certain address ranges to different units of memory, as is the convention with typical MCU memory models.

Wassasin avatar Jul 11 '22 18:07 Wassasin

A third option would be to pass a Storage unit for every MemoryUnit. This is simple as long as we only distinguish between one Internal and one (optional) External MemoryUnit, but gets more complicated as soon as there are more than two different MemoryUnits (e.g. when using two external memories).

Is the use-case with two different external memories realistic? If so, I propose we either

  • replace MemoryUnit with a number (u8 should be enough) indicating which memory to use (not very ergonomic)
  • make MemoryUnit have an Internal() and External(u8) value, where External(u8) addresses the n-th external memory

Both of these are pretty complicated and still not as ergonomic as I prefer.

About your ideas:

  1. I wouldn't want to abolish/fork the embedded_storage traits, as they will make usage of an external memory with drivers implementing the traits very easy, in addition to the handy NorFlash implementations
  2. Having a manual memory map would make it easier for moonboot, but part of my goal with this project is to make usage of the framework as ergonomic as possible (as in, integrated into rust code, if possible "autogenerated"). See for example the generation of linker scripts. Making the consumer of this lib manage a virtual memory map in addition to the microcontrollers memory-map would make things more complicated. We could of course use such a memory-map internally and put an abstraction layer over the external API, but that complicate things even further I think.

jhbruhn avatar Jul 11 '22 19:07 jhbruhn

I have moved the MemoryUnit discussion to #3

jhbruhn avatar Jul 11 '22 19:07 jhbruhn