Fw compress merge

[peterharperuk]

This is a fairly big change which enables firmware compression.

Firmware compression is enabled with a flag CYW43_ENABLE_FIRMWARE_COMPRESSION.

Firmware details and functions for reading firmware are stored in function pointers in cyw43_get_firmware_funcs_default. This can be replaced by changing the macro cyw43_get_firmware_funcs

One of these functions returns the firmware details cyw43_firmware_details_func

New firmware header files have been added that have a compressed (gzip) version of the firmware.

If firmware is compressed it reads the firmware with uzlib, see cyw43_gz_read.c

As the firmware header files are quite complicated now, I have a project to generate them. This also includes a test/demo that demonstrates how firmware could now be stored in flash, see https://github.com/peterharperuk/cyw43-firmware

[kilograham]

typedef struct cyw43_firmware_funcs {
    const cyw43_firmware_details_t* (*firmware_details)(void); ///< get wifi firmware details
    int (*start_wifi_fw)(const cyw43_firmware_details_t *fw_details); ///< start wifi firmware loading
    int (*start_bt_fw)(const cyw43_firmware_details_t *fw_details); ///< start bt firmware loading
    const uint8_t* (*get_wifi_fw)(const uint8_t *addr, size_t sz_in, uint8_t *buffer, size_t buffer_sz); ///< get block of wifi firmware data
    const uint8_t* (*get_bt_fw)(const uint8_t *addr, size_t sz_in, uint8_t *buffer, size_t buffer_sz); ///< get block of bt firmware data
    const uint8_t* (*get_nvram)(const uint8_t *addr, size_t sz_in, uint8_t *buffer, size_t buffer_sz); ///< get block of nvram data
    const uint8_t* (*get_clm)(const uint8_t *addr, size_t sz_in, uint8_t *buffer, size_t buffer_sz); ///< get block of clm data
    void (*end)(void); ///< end firmware loading
} cyw43_firmware_funcs_t;

So my initial impressions:

1. Kinda weird that the get_wifi_fw etc don't take the fw_details (or some context)
2. Moving on from 1) you would then not need separate wifi/bt/nvram/clm funcs
3. I notice that all of these get_ functions are assumed to pass sequential addresses in order, so passing the address arg seems superfluous
4. it seems oddly specific to single out wifi ant bt specially (and not nvram and clm)

Also, looking at

typedef struct cyw43_firmware_details {
    size_t raw_wifi_fw_len;     ///< Size in bytes of the combined wifi firmware data before extraction
    size_t wifi_fw_len;         ///< Size of the wifi firmware in bytes after extraction
    size_t clm_len;             ///< Size of the clm blob in bytes after extraction
    const uint8_t *wifi_fw_addr;///< Pointer to the raw wifi firmware
    const uint8_t *clm_addr;    ///< Pointer to the raw clm blob in uncompressed firmware
    size_t wifi_nvram_len;      ///< Size of nvram data
    const uint8_t *wifi_nvram_addr; ///< Pointer to nvram data
    size_t raw_bt_fw_len;       ///< size of bluetooth firmware data before extraction
    size_t bt_fw_len;           ///< size of bluetooth firmware data after extraction
    const uint8_t *bt_fw_addr;  ///< Pointer to the bluetooth firmware
} cyw43_firmware_details_t;

I wonder if some of these (e.g. wifi_fw_addr) belong in here... I think the firmware details (and I haven't totally scrutinized the firmware loading code) should be limited to meaningful data about the firmware itself (sizes and wot not)

These are just suggestions, perhaps @dpgeorge has some thoughts too

Maybe something like this

typedef struct cyw43_firmware_funcs {
    const cyw43_firmware_details_t* (*firmware_details)(void); ///< get wifi firmware details
    int  (*start_fw_stream)(const cyw43_firmware_details_t *fw_details, enum cyw43_fw_type which_firmware, void **streaming_context); ///< start fw streaming
    // returns number of bytes copied
    int  (*stream_fw)(void *streaming_context, uint8_t *buffer, size_t buffer_sz); ///< get block of firmware data
    void (*end_fw_stream)(void *streaming_context context); ///< end firmware loading
} cyw43_firmware_funcs_t;

you can still use a static streaming_context if you like (we can document that we won't read multiple fws concurrently/overlapped)

[peterharperuk]

Thanks Graham. Some comments below - but I'll have a think about your suggestion.

Kinda weird that the get_wifi_fw etc don't take the fw_details (or some context)

I guess they don't need any context. To support reading uncompressed, we just return the address requested. To support decompression we return a buffer of the size requested and the address is unused.

you would then not need separate wifi/bt/nvram/clm funcs

The only reason for having them separate is that nvram is always uncompressed. It's a small human readable separate file that's annoyingly loaded between wifi and clm data, so would need a separate decompression object.

passing the address arg seems superfluous

It is, unless compression is disabled, when it's the actual data.

oddly specific to single out wifi ant bt specially

Yes, it is a bit odd. It's because wifi and bt is loaded at different times. Maybe I should have start functions for all the components in case they are ever separated.

I wonder if some of these (e.g. wifi_fw_addr) belong in here

I'm not sure where else we'd get the data from. It's used to get a pointer to the compressed data, or the actual data if

[peterharperuk]

I might try compressing the nvram data into the wifi firmware file before clm data, so the order of compressed data matches the order the data is used. Currently I've left nvram uncompressed which doesn't work with the suggested API.

[peterharperuk]

I updated the API more or less as Graham suggested. But cyw43_firmware_details_t is unchanged.

[dpgeorge]

I haven't done a detailed review of this, but here are a few comments/questions based on an initial look:

• It looks like the uzlib source is excluded. Is that intentional? If so, the project that integrates this driver would need to provide uzlib.
• Did you see my implementation of a gz stream for a bootloader here: https://github.com/micropython/micropython/blob/master/ports/stm32/mboot/gzstream.c ? That may be a simpler implementation that can be used.
• Do you have any rough numbers of how much space is saved with compression enabled (once all the overhead, including uzlib, is taken into account)?
• How much slower is it to load the firmware when it's compressed?
• Why is it so complicated to generate the compressed firmware header files? Can it be done in a few lines of Python?

[peterharperuk]

It looks like the uzlib source is excluded. Is that intentional? If so, the project that integrates this driver would need to provide uzlib.

I had to make sure the pico-sdk version of uzlib was not included as there is already a version in Micropython added in moduzlib.c. There didn't seem to be a nicer way to solve this.

Did you see my implementation of a gz stream for a bootloader here: https://github.com/micropython/micropython/blob/master/ports/stm32/mboot/gzstream.c ? That may be a simpler implementation that can be used.

It looks similar to the code I have - I suspect I copied it?

Do you have any rough numbers of how much space is saved with compression enabled (once all the overhead, including uzlib, is taken into account)?

There are two versions of wifi firmware,

just wifi...
without compression: 225240 bytes
with compression: 143491 bytes, saving 80KB (64%)

bt+wifi...
without compression: 232408 bytes
with compression: 148992 bytes, saving 81KB (64%)

Just the bt firmware...
without compression: 35182 bytes
with compression: 25457, saving 9KB (72%)

How much slower is it to load the firmware when it's compressed?

I'll have to measure that and get back to you.

Why is it so complicated to generate the compressed firmware header files? Can it be done in a few lines of Python?

It's just complicated to convert the raw binaries into header files and add the "meta" data needed. I could write it in Python but I thought make would be easier - see https://github.com/peterharperuk/cyw43-firmware/blob/master/make_firmware/Makefile

[peterharperuk]

Some performance measurements... (still to do bluetooth)

Firmware loading without any changes: wifi 135.95ms nvram 3.68ms, clm 3.64ms
Firmware loading with these changes but uncompressed firmware: wifi 127.95ms, nvram 3.71ms, clm 3.64ms
Firmware loading with compressed firmware: wifi 792.89ms, nvram 3.80, clm 6.04ms

So in summary, wifi +657ms, nvram +0.12ms, clm +2.4ms

[dpgeorge]

Firmware loading with compressed firmware: wifi 792.89ms

That's quite a big increase in time taken. Is this for Pico W? I already notice that Pico W is rather slow doing network.WLAN().active(1) (using MicroPython), that takes about 870ms. I'm not sure exactly where all that time goes but I was guessing it goes to download the wifi firmware over a single data line.

If compression approximately doubles that time to activate the WLAN interface, that will definitely be noticed by users.

[peterharperuk]

Yes, Pico W. The cyw43 firmware is 227KB and is downloaded in 64 byte chunks for some reason. I'm using max compression. I guess we could speed it up by compressing less, but it's always going to be slower. We could even disable compression by default.

[dpgeorge]

It's a difficult trade off to decide upon: save 80k of flash but pay 650ms penalty?

[peterharperuk]

I had a play around with the compression ratio. It doesn't seem to make any difference to the decompression time. Increasing the dictionary size from 4KB to 32KB reduces the load time from >800 to 348ms, so around 212ms penalty for compression.