gh-raspberrypi/pico-sdk

Watch

Fork 0

Flash bank customisation #1293

Merged

peterharperuk merged 2 commits from bt_flash_bank_dynamic into develop

2023-06-06 12:48:10 -05:00

peterharperuk commented

2023-03-01 09:56:55 -06:00

(Migrated from github.com)

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 #1278

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 #1278

kilograham commented

2023-03-01 13:09:13 -06:00

(Migrated from github.com)

Frankly, i'd just be tempted to use __builtin_constant_p() in an ifdef around the static assertions

Frankly, i'd just be tempted to use `__builtin_constant_p()` in an ifdef around the static assertions

peterharperuk commented

2023-03-02 04:54:59 -06:00

(Migrated from github.com)

Nice. I literally didn't know that existed

peterharperuk commented

2023-03-02 06:39:01 -06:00

(Migrated from github.com)

I've used __builtin_constant_p()
Not sure what you meant by using it in a ifdef as it's a compiler not preprocessor thing?

I've used __builtin_constant_p() Not sure what you meant by using it in a ifdef as it's a compiler not preprocessor thing?

kilograham commented

2023-03-02 09:47:37 -06:00

(Migrated from github.com)

Not sure what you meant by using it in a ifdef as it's a compiler not preprocessor thing?

uh yeah, duh, good point ;-) too little sleep!

> Not sure what you meant by using it in a ifdef as it's a compiler not preprocessor thing? uh yeah, duh, good point ;-) too little sleep!

peterharperuk commented

2023-03-03 05:00:08 -06:00

(Migrated from github.com)

Unless I'm mistaken this still isn't going to work without modifying files in the sdk?

They want to define the flash bank storage offset with pointer arithmetic. Which means the variable has to be defined in the btstack_flash_bank.h file. It can't go in pico_w.h because that's included in assembler.

Maybe we just need to make it possible to use an alternate version of pico_flash_bank_instance maybe via a #define specifying the function name or a cmake variable specifying a different source file. @kilograham are there any other examples for enabling customisation like this?

Unless I'm mistaken this still isn't going to work without modifying files in the sdk? They want to define the flash bank storage offset with pointer arithmetic. Which means the variable has to be defined in the btstack_flash_bank.h file. It can't go in pico_w.h because that's included in assembler. Maybe we just need to make it possible to use an alternate version of pico_flash_bank_instance maybe via a #define specifying the function name or a cmake variable specifying a different source file. @kilograham are there any other examples for enabling customisation like this?

peterharperuk commented

2023-03-03 06:20:03 -06:00

(Migrated from github.com)

I've fixed this differently. Now if you define pico_flash_bank_get_offset_func you can change the function used to get the offset.
Some example code here github.com/peterharperuk/pico-examples@4f26716e75

I've fixed this differently. Now if you define pico_flash_bank_get_offset_func you can change the function used to get the offset. Some example code here https://github.com/peterharperuk/pico-examples/commit/4f26716e7535a6b260ed0c32cc5e6a2fe8892c28

peterharperuk commented

2023-03-03 06:20:46 -06:00

(Migrated from github.com)

@earlephilhower you better tell me if you're happy with this.

earlephilhower commented

2023-03-03 09:39:32 -06:00

(Migrated from github.com)

@peterharperuk, first thanks a bunch for your support. That's a neat solution ~~, but is it safe to assume the size in the SDK when this is called? (I assume you'll need to adjust the SDK to call this as well?)~~ --edit-- I see it's still a `#define, ignore this question --edit--

I'm also thinking, maybe this and https://github.com/raspberrypi/pico-sdk/issues/1278 can also be dropped. The reason is that the current TVL flash bank implementation isn't multicore safe, and I think lots of folks are going to use that 2nd core. The SDK can't really cater to all use cases, but it can be a simple example for folks to build off of for their own TLV implementations, maybe?

@peterharperuk, first thanks a bunch for your support. That's a neat solution ~~, but is it safe to assume the size in the SDK when this is called? (I assume you'll need to adjust the SDK to call this as well?)~~ --edit-- I see it's still a `#define, ignore this question --edit-- I'm also thinking, maybe this and https://github.com/raspberrypi/pico-sdk/issues/1278 can also be dropped. The reason is that the current TVL flash bank implementation isn't multicore safe, and I think lots of folks are going to use that 2nd core. The SDK can't really cater to all use cases, but it can be a simple example for folks to build off of for their own TLV implementations, maybe?

peterharperuk commented

2023-03-03 09:47:50 -06:00

(Migrated from github.com)

I don't think you should be using sizeof to set the size of the flash area. I can't see the need. Just set PICO_FLASH_BANK_TOTAL_SIZE to the number of sectors you want to reserve.

Are you saying you don't need this change? If the flash bank implementation isn't multicore safe (you're probably right) we can just fix that.

I don't think you should be using sizeof to set the size of the flash area. I can't see the need. Just set PICO_FLASH_BANK_TOTAL_SIZE to the number of sectors you want to reserve. Are you saying you don't need this change? If the flash bank implementation isn't multicore safe (you're probably right) we can just fix that.

earlephilhower commented

2023-03-03 10:13:57 -06:00

(Migrated from github.com)

I don't think you should be using sizeof to set the size of the flash area. I can't see the need. Just set PICO_FLASH_BANK_TOTAL_SIZE to the number of sectors you want to reserve.

Agreed, I edited afterwards which probably wasn't obvious once I realized the same thing. Sorry, it was pre-coffee in the morning for me!

Are you saying you don't need this change? If the flash bank implementation isn't multicore safe (you're probably right) we can just fix that.

Well, this is necessary but not sufficient for my use case. The multicore safety needs to be there, too, but then I think I could use the stock version, yes.

But, my comment was more that you're moving into the application-side of things here, while most of the SDK is low-level tools. TLV could be in a fixed flash sector, it could be in a dynamic one like this patch, it could be a file (if you have a FS you could spread out flash wear), etc. So you've shown how to do it already, and since BT is really an app-level thing it's up to the end user to implement the right TLV store for their app.

> I don't think you should be using sizeof to set the size of the flash area. I can't see the need. Just set PICO_FLASH_BANK_TOTAL_SIZE to the number of sectors you want to reserve. Agreed, I edited afterwards which probably wasn't obvious once I realized the same thing. Sorry, it was pre-coffee in the morning for me! > Are you saying you don't need this change? If the flash bank implementation isn't multicore safe (you're probably right) we can just fix that. Well, this is necessary but not sufficient for my use case. The multicore safety needs to be there, too, but then I think I could use the stock version, yes. But, my comment was more that you're moving into the application-side of things here, while most of the SDK is low-level tools. TLV could be in a fixed flash sector, it could be in a dynamic one like this patch, it could be a file (if you have a FS you could spread out flash wear), etc. So you've shown how to do it already, and since BT is really an app-level thing it's up to the end user to implement the right TLV store for their app.

peterharperuk commented

2023-03-03 13:23:21 -06:00

(Migrated from github.com)

Understood. I agree we're straying into the app side of things. We just want to provide the minimum necessary for the users who just want their code to work. And we don't want to get in the way of "power users" like yourself. I'll look into the multicore safety issue as that was an open issue that needs to be resolved.

👍 1

kilograham (Migrated from github.com) reviewed

2023-03-19 16:43:03 -05:00

kilograham (Migrated from github.com) left a comment

seems like this should be documented somewhere (either PICO_CONFIG, or perhaps in the header)

src/rp2_common/pico_btstack/include/pico/btstack_flash_bank.h

you can't put a comma in the description; it breaks the doc tooling

peterharperuk (Migrated from github.com) reviewed

2023-03-23 06:57:00 -05:00

src/rp2_common/pico_btstack/include/pico/btstack_flash_bank.h

I think you were looking at an old version of the change? PICO_FLASH_BANK_FIXED_OFFSET doesn't exist anymore. Instead I use pico_flash_bank_get_offset_func