raburton / rboot

An open source bootloader for the ESP8266
https://richard.burtons.org/tag/rboot/?order=ASC
MIT License
300 stars 72 forks source link

Read firmware with bigger chunks and without temp buffer #24

Closed djphoenix closed 7 years ago

djphoenix commented 7 years ago

I see that in stage2 you're use temporary buffer of size 0x100. The reason of this is unknown for me, but with this patch it is keeps to work for me (much faster):

diff --git a/rboot-stage2a.c b/rboot-stage2a.c
index 9df98a8..2b2a5a4 100644
--- a/rboot-stage2a.c
+++ b/rboot-stage2a.c
@@ -42,13 +42,10 @@ usercode* NOINLINE load_rom(uint32 readpos) {

                while (remaining > 0) {
                        // work out how much to read, up to 16 bytes at a time
-                       uint32 readlen = (remaining < BUFFER_SIZE) ? remaining : BUFFER_SIZE;
+                       uint32 readlen = (remaining < 0x1000) ? remaining : 0x1000;
                        // read the block
-                       SPIRead(readpos, buffer, readlen);
+                       SPIRead(readpos, writepos, readlen);
                        readpos += readlen;
-                       // copy the block
-                       ets_memcpy(writepos, buffer, readlen);
-                       // increment next write position
                        writepos += readlen;
                        // decrement remaining count
                        remaining -= readlen;

Can you describe why you're use this buffer?

And, so, with union you can keep some space if one of header is less than 0x100 bytes:

diff --git a/rboot-stage2a.c b/rboot-stage2a.c
index 9df98a8..c9fd1cb 100644
--- a/rboot-stage2a.c
+++ b/rboot-stage2a.c
@@ -13,14 +13,18 @@

 usercode* NOINLINE load_rom(uint32 readpos) {

-   uint8 buffer[BUFFER_SIZE];
+   union {
+       rom_header rom;
+       section_header section;
+   } buffer;

        uint8 sectcount;
        uint8 *writepos;
        uint32 remaining;
        usercode* usercode;

-   rom_header *header = (rom_header*)buffer;
-   section_header *section = (section_header*)buffer;
+   rom_header *header = &buffer.rom;
+   section_header *section = &buffer.section;

        // read rom header
        SPIRead(readpos, header, sizeof(rom_header));
raburton commented 7 years ago

I have previously tried larger sizes and it didn't work properly. The buffer is just there for historic reasons, but I can't imagine it's harming the performance much. As for saving space, yes if the buffer were removed you would want to tidy up that bit of code a bit, but saving space is not a concern - we have the entire devices memory to play with, there is nothing else running. The code would be nearer and clearer not having any kind of union or shared buffer at all, just have two separate structures.

djphoenix commented 7 years ago

So for direct in-memory read, it works for 4k size (so, not tried larger, but I think sector-read is safe). About buffer - yes, I think it is safe to get two buffers for rom/section headers (gcc should optimise them on stack anyway). Should I prepare a pull request, or you may implement it in your way?

raburton commented 7 years ago

PR would be nice please. I currently have no development system, so I'll have to take your word for it that's it's working fine so please test carefully.