Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't use non-absolute offset for Bluetooth TLV storage area in PICO_FLASH_BANK #1278

Closed
earlephilhower opened this issue Feb 23, 2023 · 3 comments
Assignees
Milestone

Comments

@earlephilhower
Copy link
Contributor

earlephilhower commented Feb 23, 2023

By default the 2 Bluetooth TLV stores are placed at the last 2 sectors of flash.

Unfortunately, in my own framework many people already have data in those last two sectors, so I need to move them elsewhere. Dynamically would be best. So I'm attempting to do something like creating a "const" variable that can be overwritten at the proper alignment in app code

const uint8_t __bluetooth_tlv[8192] __attribute__ ((aligned(4096))) = { 0 };
extern const uint8_t __flash_binary_start;

#define PICO_FLASH_BANK_TOTAL_SIZE sizeof(__bluetooth_tlv)
#define PICO_FLASH_BANK_STORAGE_OFFSET ((unsigned int)(__bluetooth_tlv - &__flash_binary_start))

(the undef/redef in the real app is handled in the Cmakefile and btstack_config.h, this is just a self-contained snippet for simplicity)

The problem is that the following static asserts fail due to the "non-constness" of the address-of operator. The compiler can't know the offset of the variable, only the linker knows that, so the asserts fail.

static_assert(PICO_FLASH_BANK_TOTAL_SIZE % (FLASH_SECTOR_SIZE * 2) == 0, "PICO_FLASH_BANK_TOTAL_SIZE invalid");
static_assert(PICO_FLASH_BANK_TOTAL_SIZE <= PICO_FLASH_SIZE_BYTES, "PICO_FLASH_BANK_TOTAL_SIZE too big");
static_assert(PICO_FLASH_BANK_STORAGE_OFFSET + PICO_FLASH_BANK_TOTAL_SIZE <= PICO_FLASH_SIZE_BYTES, "PICO_FLASH_BANK_TOTAL_SIZE too big");

I would ask to either remove those static_asserts or bracket them with, say, a new marker #if !defined(PICO_FLASH_BANK_DYNAMIC). There doesn't seem to be any further dependency on the values being compile-time constants, only these assertions.

-- edit --
One additional runtime assert needs to be bracketed or removed as well (it's checking we don't overwrite the "app" with the flash TLV, but in this use case we actually expect to):

assert((uintptr_t)&__flash_binary_end - XIP_BASE <= PICO_FLASH_BANK_STORAGE_OFFSET);

@kilograham kilograham added this to the 1.5.1 milestone Feb 23, 2023
@peterharperuk
Copy link
Contributor

peterharperuk commented Feb 24, 2023

It sounds like we should just make it runtime rather than build time configurable?
Edit. Actually ignore that. I see what you're doing now. I'm fine with changing the library to use your method if it works. I'd rather not add more ifdef complications.

peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Mar 1, 2023
Defaults to true. Can be set to false if the flash bank offset is
dynamically generated, e.g. to be inside the binary itself.

Fixes raspberrypi#1278
peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Mar 2, 2023
Fix static asserts so they don't fail to compile if the flash bank
location is inside the binary itself.

Fixes raspberrypi#1278
peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Mar 3, 2023
Allow the pico_flash_bank_get_offset function to be changed by
defining pico_flash_bank_get_offset_func

Fixes raspberrypi#1278
peterharperuk added a commit to peterharperuk/pico-examples that referenced this issue Mar 3, 2023
peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Mar 3, 2023
Allow the pico_flash_bank_get_offset function to be changed by
defining pico_flash_bank_get_offset_func

Fixes raspberrypi#1278
peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Jun 6, 2023
Allow the pico_flash_bank_get_offset function to be changed by
defining pico_flash_bank_get_offset_func

Fixes raspberrypi#1278
peterharperuk added a commit to peterharperuk/pico-sdk that referenced this issue Jun 6, 2023
Allow the pico_flash_bank_get_offset function to be changed by
defining pico_flash_bank_get_offset_func

Fixes raspberrypi#1278
@kilograham
Copy link
Contributor

merged into develop

@earlephilhower
Copy link
Contributor Author

Thanks, @kilograham and @peterharperuk !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants