Open eriksl opened 5 years ago
It's so long since I looked at this code I'm struggling to remember what the old code did and so then what new code does! I'm sure this is fine, but going to think about this a little more and see what comes back to me...
I can figure that, it took me some experimenting and crashing before I realised what's really going on here.
What my code version of the code does is mostly the same of the original code but then it also checks the signature. I also tried adding checking the checksum, but it will add to much IRAM usage weighted to what it adds.
The main differences is that the original code uses pointer arithmetic and bitmask operations, which is robust but imho difficult to read for humans and therefore prone to error. I replaced it with an overlay struct that accesses the same bitfields and, imho, is easier to understand as to what it exactly does.
But as you have a little more "users" than I do, I'd rather have this reviewed well or not applied. In that case I will keep it in my fork.
This appears to copy 16 bytes, although the structure is only 9 including the checksum byte we aren't checking. On the other hand, if anyone were to add to the rtc data structure they'd need to know to come here and increase this size if they exceeded it. I know you don't like the manipulations of the bits, but it's certainly more efficient in terms of data size and operations. Ultimately you just want to check the magic value and i think that first reading that and then if ok reading the second value (as we currently do) or getting from the conf as per your code, would be a lot more efficient (though not as readable).
Something like this:
+ uint8_t off = (uint8_t*)&rtc.last_rom - (uint8_t*)&rtc;
+ // check the magic
+ volatile uint32_t *rtcd = (uint32_t*)(0x60001100 + (RBOOT_RTC_ADDR*4));
+ if (*rtcd != RBOOT_RTC_MAGIC) {
+ val = conf.roms[conf.current_rom];
+ } else {
+ // get the four bytes containing the one of interest
+ rtcd = (uint32_t*)(0x60001100 + (RBOOT_RTC_ADDR*4) + (off & ~3));
+ val = *rtcd;
+ // extract the one of interest
+ val = ((uint8_t*)&val)[off & 3];
+ // get address of rom
+ val = conf.roms[val];
+ }
What my code does is copy the struct from I/O address space to RAM, neatly aligned in chunks of four bytes. After that you can access any of the struct fields in any way. But of course it's not required to do that way. I think this way is already way better readable.
In the meantime I created my own version inside the project, I am not using this one anymore, as I am only using it in a very limited way and that spares me IRAM bytes. It's now using 88 bytes in total.
So you now can do whatever you like with this function ;-)
BTW two things that might have slipped your attention:
I wonder if we could get away by simply not caring about the rest of the sector. I.e. read sector up to size of config sector, erase sector and then only write sector only up to config sector size. No memory buffer required then. The only thing I am not sure of is how flash write behaves on a size not a multiple of 4k. According to the docs it should the_right_thing, but hey, it's espressif, who knows.
I now made an alternative api function that does the thing with the supplied memory block, so I am not sure if my other suggestion will work.
That makes my code again pvPortMalloc/Free "free", which I prefer and greatly contributes to stability.
This appears to copy 16 bytes, although the structure is only 9 including the checksum byte we aren't checking. On the other hand, if anyone were to add to the rtc data structure they'd need to know to come here and increase this size if they exceeded it.
Today I realised this fact as well and added an _Static_assert that checks if the overlay is always bigger than the rtc data.
_Static_assert(sizeof(rboot_if_rtc_config_t) < sizeof(uint32_t[4]), "RTC RAM struct overlay too small");
Unfortunately there doesn't seem to be a way to apply this to the struct fields directly, which would be cleaner.
The current implementation doesn't check the magic number. If rboot is not yet updated for RTC mode (and your image IS), it will crash.
Also I think this code is cleaner.
It will add a few bytes to IRAM usage though.