__not_in_flash_func() requested around rom_func_lookup() and others.

smithps commented

2021-05-14 08:54:01 -05:00

(Migrated from github.com)

Hi guys,
I think I'm pushing the boundaries a little here which is why I have created the issue I have.

Here's the background;

I've ported an app from an Adafruit Arduino M0 board with an SD card connected to it.
The Arduino had the facility to put a UF2 file on the SD card and it would reflash itself in the same way that on the Pico you can drop the UF2 file on the drive that appears when you plug in the USB.
I wanted to emulate what the Adafruit board did, so I have written some code which goes into the startup of my app and does the following;

Initialises an SD card interface using SPI.
Looks for a UF2 file
If found, processes the contents of the UF2 file to a base address half way up the flash (ignoring the addresses specified in the UF2 file itself)
Now running from code based in RAM, it moves the processed data back down to the Flash base address and then resets the device.

So long as all of my apps contain the same code, I can reprogram the device just by putting the UF2 image on the SD card and inserting it.

The issue I seem to have caused myself is that calling the flash routines in the code below causes the app to crash because there are certain dependent functions that are in the flash I'm writing to.

`
for (size_t PageCount = 0; PageCount < NewNumBlocks; PageCount++)
{
uint8_t DataBlock[FLASH_PAGE_SIZE];

	// Sectors are 4096 bytes, so erase sectors at a time...
	if((PageCount % 16) == 0)
		flash_range_erase((uint32_t)(PageCount * FLASH_PAGE_SIZE), FLASH_SECTOR_SIZE);
					
	// Pages are 256 bytes...
	// Copy from the destination FLASH address into the DataBlock[] in RAM.
	for (int i = 0 ; i < FLASH_PAGE_SIZE ; i++)
		DataBlock[i] = *pNewFW++;
					
	// Now program from DataBlock[] in RAM to new address in FLASH.
	flash_range_program(OriginalFWOffset, &DataBlock[0], FLASH_PAGE_SIZE);
					
	OriginalFWOffset += FLASH_PAGE_SIZE;
}

`
It appears that flash_range_erase() and flash_range_program() are declared __no_inline_not_in_flash_func, but the dependencies within them are not.

I can fix the problem by changing the declarations of the functions below to be __not_in_flash_func()

rom_func_lookup (bootrom.c)
rom_funcs_lookup (bootrom.c)
__compiler_memory_barrier (platform.h)
rom_table_code (bootrom.h)

My questions are;

Is that the right way to fix the problem?
I'm not entirely sure what __compiler_memory_barrier does, so do I risk "breaking" that?
should these anciliary functions already be declared to be not in flash anyway?

P.S. I do have this all working, but I'm after clarification as to whether or not what I am doing to delcare these additional functions as not in flash is correct and should be ported into the standard SDK or not.

Hi guys, I think I'm pushing the boundaries a little here which is why I have created the issue I have. Here's the background; - I've ported an app from an Adafruit Arduino M0 board with an SD card connected to it. - The Arduino had the facility to put a UF2 file on the SD card and it would reflash itself in the same way that on the Pico you can drop the UF2 file on the drive that appears when you plug in the USB. - I wanted to emulate what the Adafruit board did, so I have written some code which goes into the startup of my app and does the following; 1. Initialises an SD card interface using SPI. 2. Looks for a UF2 file 3. If found, processes the contents of the UF2 file to a base address half way up the flash (ignoring the addresses specified in the UF2 file itself) 4. Now running from code based in RAM, it moves the processed data back down to the Flash base address and then resets the device. So long as all of my apps contain the same code, I can reprogram the device just by putting the UF2 image on the SD card and inserting it. The issue I seem to have caused myself is that calling the flash routines in the code below causes the app to crash because there are certain dependent functions that are in the flash I'm writing to. ` for (size_t PageCount = 0; PageCount < NewNumBlocks; PageCount++) { uint8_t DataBlock[FLASH_PAGE_SIZE]; // Sectors are 4096 bytes, so erase sectors at a time... if((PageCount % 16) == 0) flash_range_erase((uint32_t)(PageCount * FLASH_PAGE_SIZE), FLASH_SECTOR_SIZE); // Pages are 256 bytes... // Copy from the destination FLASH address into the DataBlock[] in RAM. for (int i = 0 ; i < FLASH_PAGE_SIZE ; i++) DataBlock[i] = *pNewFW++; // Now program from DataBlock[] in RAM to new address in FLASH. flash_range_program(OriginalFWOffset, &DataBlock[0], FLASH_PAGE_SIZE); OriginalFWOffset += FLASH_PAGE_SIZE; } ` It appears that flash_range_erase() and flash_range_program() are declared **__no_inline_not_in_flash_func**, but the dependencies within them are not. I can fix the problem by changing the declarations of the functions below to be **__not_in_flash_func()** - rom_func_lookup (bootrom.c) - rom_funcs_lookup (bootrom.c) - __compiler_memory_barrier (platform.h) - rom_table_code (bootrom.h) My questions are; 1. Is that the right way to fix the problem? 2. I'm not entirely sure what __compiler_memory_barrier does, so do I risk "breaking" that? 3. should these anciliary functions already be declared to be not in flash anyway? P.S. I do have this all working, but I'm after clarification as to whether or not what I am doing to delcare these additional functions as **not in flash** is correct and should be ported into the standard SDK or not.

lurch commented

2021-05-14 09:17:07 -05:00

(Migrated from github.com)

Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as __not_in_flash_func should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? 🤷

Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as `__not_in_flash_func` should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? :shrug:

kilograham commented

2021-05-14 10:04:27 -05:00

(Migrated from github.com)

Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as __not_in_flash_func should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? 🤷

Can't do that

Is that the right way to fix the problem?

That's not ideal... and perhaps this is an outside use case (since the methods are safe to call from flash unless you are overwriting the binary as you say). it is probably better if they were inline in the header (they are very simple), but they may be called from ASM also. (I'll take a look in a bit)

As a workaround you could certainly just copy the code for all these few functions.

> Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as `__not_in_flash_func` should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? 🤷 Can't do that > Is that the right way to fix the problem? That's not ideal... and perhaps this is an outside use case (since the methods are safe to call from flash unless you are overwriting the binary as you say). it is probably better if they were inline in the header (they are very simple), but they may be called from ASM also. (I'll take a look in a bit) As a workaround you could certainly just copy the code for all these few functions.

smithps commented

2021-05-14 10:05:07 -05:00

(Migrated from github.com)

Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as __not_in_flash_func should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? 🤷

I do see what you mean, and recursiveness was also my thought. Currently seems like it's a manual job to go through your code and seek out all those functions that are called. Wouldn't affect too many users I don't think - after all, erasing the code you are running is not a normal activity!

> Interesting... I dunno how compiler-internals and these function attributes work, but I wonder if any functions marked as `__not_in_flash_func` should "recursively" be not-in-flash automatically (IYSWIM)? Or perhaps that's simply something that the compiler can't do automatically? 🤷 I do see what you mean, and recursiveness was also my thought. Currently seems like it's a manual job to go through your code and seek out all those functions that are called. Wouldn't affect too many users I don't think - after all, erasing the code you are running is not a normal activity!

kilograham commented

2021-05-14 10:08:35 -05:00

(Migrated from github.com)

Note also you could just replace pico_bootrom by setting SKIP_PICO_BOOTROM=1 at the top of your CMakeLists.txt, and adding your own pico_bootrom INTERFACE library which does the same thing but with __not_in_flash_func

Note also you could just replace _pico_bootrom_ by setting `SKIP_PICO_BOOTROM=1` at the top of your CMakeLists.txt, and adding your own _pico_bootrom_ INTERFACE library which does the same thing but with `__not_in_flash_func`

👍 1

smithps commented

2021-05-14 10:11:37 -05:00

(Migrated from github.com)

As a workaround you could certainly just copy the code for all these few functions.

Yes, that works fine - it just doesn't seem elegant to me - if someone changes any of these functions in future SDK versions I'd have to hunt them down and make sure the changes got applied to my copied functions.....having said that, I'd still have to modify the SDK to put the __not_in_flash_func macros back.

I'm now going to read up on the suggestion of setting SKIP_PICO_BOOTROM and making a new INTERFACE library.

> As a workaround you could certainly just copy the code for all these few functions. Yes, that works fine - it just doesn't seem elegant to me - if someone changes any of these functions in future SDK versions I'd have to hunt them down and make sure the changes got applied to my copied functions.....having said _that_, I'd still have to modify the SDK to put the __not_in_flash_func macros back. I'm now going to read up on the suggestion of setting SKIP_PICO_BOOTROM and making a new INTERFACE library.

smithps commented

2021-05-14 10:47:14 -05:00

(Migrated from github.com)

Note also you could just replace pico_bootrom by setting SKIP_PICO_BOOTROM=1 at the top of your CMakeLists.txt, and adding your own pico_bootrom INTERFACE library which does the same thing but with __not_in_flash_func

I must be being thick.... I can't find 'SKIP_PICO_BOOTROM' in any of the SDK or any documentation. Can you advise please?

> Note also you could just replace _pico_bootrom_ by setting `SKIP_PICO_BOOTROM=1` at the top of your CMakeLists.txt, and adding your own _pico_bootrom_ INTERFACE library which does the same thing but with `__not_in_flash_func` I must be being thick.... I can't find 'SKIP_PICO_BOOTROM' in any of the SDK or any documentation. Can you advise please?

lurch commented

2021-05-14 10:58:20 -05:00

(Migrated from github.com)

I can't find 'SKIP_PICO_BOOTROM' in any of the SDK or any documentation.

https://github.com/raspberrypi/pico-sdk/blob/develop/src/CMakeLists.txt#L21 ?

> I can't find 'SKIP_PICO_BOOTROM' in any of the SDK or any documentation. https://github.com/raspberrypi/pico-sdk/blob/develop/src/CMakeLists.txt#L21 ?

👍 1

smithps commented

2021-05-14 11:54:55 -05:00

(Migrated from github.com)

Note also you could just replace pico_bootrom by setting SKIP_PICO_BOOTROM=1 at the top of your CMakeLists.txt, and adding

I presume you meant set(SKIP_PICO_BOOTROM 1), which is what I have actually done. Thanks for the tip. I'd like to add that I couldn't find any documentation of this nice feature anywhere. It's use might make a nice example for the Pico examples too.

> Note also you could just replace _pico_bootrom_ by setting `SKIP_PICO_BOOTROM=1` at the top of your CMakeLists.txt, and adding I presume you meant **set(SKIP_PICO_BOOTROM 1)**, which is what I have actually done. Thanks for the tip. I'd like to add that I couldn't find any documentation of this nice feature anywhere. It's use might make a nice example for the Pico examples too.

kilograham commented

2021-05-14 12:16:07 -05:00

(Migrated from github.com)

Yes; it isn't currently documented - feel free to make a PR in pico-examples with an example ;-)

Wren6991 commented

2021-05-17 03:48:15 -05:00

(Migrated from github.com)

It appears that flash_range_erase() and flash_range_program() are declared __no_inline_not_in_flash_func, but the dependencies within them are not.

Even if its callees are in RAM, the flash.c code explicitly does data access on flash on non-NO_FLASH builds, when it takes a copy of the second stage bootloader (flash_init_boot2_copyout()).

Looking at flash_range_erase as an example:

void __no_inline_not_in_flash_func(flash_range_erase)(uint32_t flash_offs, size_t count) {
#ifdef PICO_FLASH_SIZE_BYTES
    hard_assert(flash_offs + count <= PICO_FLASH_SIZE_BYTES);
#endif
    invalid_params_if(FLASH, flash_offs & (FLASH_SECTOR_SIZE - 1));
    invalid_params_if(FLASH, count & (FLASH_SECTOR_SIZE - 1));
    void (*connect_internal_flash)(void) = (void(*)(void))rom_func_lookup(rom_table_code('I', 'F'));
    void (*flash_exit_xip)(void) = (void(*)(void))rom_func_lookup(rom_table_code('E', 'X'));
    void (*flash_range_erase)(uint32_t, size_t, uint32_t, uint8_t) =
        (void(*)(uint32_t, size_t, uint32_t, uint8_t))rom_func_lookup(rom_table_code('R', 'E'));
    void (*flash_flush_cache)(void) = (void(*)(void))rom_func_lookup(rom_table_code('F', 'C'));
    assert(connect_internal_flash && flash_exit_xip && flash_range_erase && flash_flush_cache);
    flash_init_boot2_copyout();

    // No flash accesses after this point
    __compiler_memory_barrier();

    connect_internal_flash();
    flash_exit_xip();
    flash_range_erase(flash_offs, count, FLASH_BLOCK_SIZE, FLASH_BLOCK_ERASE_CMD);
    flash_flush_cache(); // Note this is needed to remove CSn IO force as well as cache flushing
    flash_enable_xip_via_boot2();
}

The logic of the code is:

Do a number of potentially flash-touching functions including taking a copy of the flash-resident second stage
Barrier to enforce (and really to show visually) that the flash accesses mustn't be reordered into the following code
Do some XIP-breaking things to the flash interface in order to program the flash
Use the copy of the flash second stage which was taken earlier to re-enter XIP mode

So the function is declared as __no_inline_not_in_flash_func not because it is expected to be callable when XIP of the current image is not available, but so that this critical section:

    connect_internal_flash();
    flash_exit_xip();
    flash_range_erase(flash_offs, count, FLASH_BLOCK_SIZE, FLASH_BLOCK_ERASE_CMD);
    flash_flush_cache(); // Note this is needed to remove CSn IO force as well as cache flushing
    flash_enable_xip_via_boot2();

Is able to run without flash execution. The code still expects XIP to be available when it is called, and this is the assumption that's broken when you start writing to the program image you called your RAM-resident code from.

I can fix the problem by changing the declarations of the functions below to be __not_in_flash_func()

rom_func_lookup (bootrom.c)
rom_funcs_lookup (bootrom.c)
__compiler_memory_barrier (platform.h)
rom_table_code (bootrom.h)

Honestly I'm a little surprised that those calls weren't inlined, the function lookups seem like they would benefit from it? Same for __compiler_memory_barrier, which shouldn't actually produce any code, and was a macro at one point.

> It appears that flash_range_erase() and flash_range_program() are declared __no_inline_not_in_flash_func, but the dependencies within them are not. Even if its callees are in RAM, the `flash.c` code explicitly does data access on flash on non-`NO_FLASH` builds, when it takes a copy of the second stage bootloader (`flash_init_boot2_copyout()`). Looking at `flash_range_erase` as an example: ``` void __no_inline_not_in_flash_func(flash_range_erase)(uint32_t flash_offs, size_t count) { #ifdef PICO_FLASH_SIZE_BYTES hard_assert(flash_offs + count <= PICO_FLASH_SIZE_BYTES); #endif invalid_params_if(FLASH, flash_offs & (FLASH_SECTOR_SIZE - 1)); invalid_params_if(FLASH, count & (FLASH_SECTOR_SIZE - 1)); void (*connect_internal_flash)(void) = (void(*)(void))rom_func_lookup(rom_table_code('I', 'F')); void (*flash_exit_xip)(void) = (void(*)(void))rom_func_lookup(rom_table_code('E', 'X')); void (*flash_range_erase)(uint32_t, size_t, uint32_t, uint8_t) = (void(*)(uint32_t, size_t, uint32_t, uint8_t))rom_func_lookup(rom_table_code('R', 'E')); void (*flash_flush_cache)(void) = (void(*)(void))rom_func_lookup(rom_table_code('F', 'C')); assert(connect_internal_flash && flash_exit_xip && flash_range_erase && flash_flush_cache); flash_init_boot2_copyout(); // No flash accesses after this point __compiler_memory_barrier(); connect_internal_flash(); flash_exit_xip(); flash_range_erase(flash_offs, count, FLASH_BLOCK_SIZE, FLASH_BLOCK_ERASE_CMD); flash_flush_cache(); // Note this is needed to remove CSn IO force as well as cache flushing flash_enable_xip_via_boot2(); } ``` The logic of the code is: * Do a number of potentially flash-touching functions including taking a copy of the flash-resident second stage * Barrier to enforce (and really to show visually) that the flash accesses mustn't be reordered into the following code * Do some XIP-breaking things to the flash interface in order to program the flash * Use the copy of the flash second stage which was taken earlier to re-enter XIP mode So the function is declared as `__no_inline_not_in_flash_func` not because it is expected to be callable when XIP of the current image is not available, but so that this critical section: ``` connect_internal_flash(); flash_exit_xip(); flash_range_erase(flash_offs, count, FLASH_BLOCK_SIZE, FLASH_BLOCK_ERASE_CMD); flash_flush_cache(); // Note this is needed to remove CSn IO force as well as cache flushing flash_enable_xip_via_boot2(); ``` Is able to run without flash execution. The code still expects XIP to be available when it is called, and this is the assumption that's broken when you start writing to the program image you called your RAM-resident code from. > I can fix the problem by changing the declarations of the functions below to be __not_in_flash_func() > > rom_func_lookup (bootrom.c) > rom_funcs_lookup (bootrom.c) > __compiler_memory_barrier (platform.h) > rom_table_code (bootrom.h) Honestly I'm a little surprised that those calls weren't inlined, the function lookups seem like they would benefit from it? Same for `__compiler_memory_barrier`, which shouldn't actually produce any code, and was a macro at one point.

Wren6991 commented

2021-05-17 03:56:53 -05:00

(Migrated from github.com)

It's a little unconventional for a program to overwrite its own image. More commonly, this kind of "move the world" operation is a separate, self-contained bootloader. That way you get better linker hygiene between the code that might be in flash and the code that mustn't be.

👍 1

smithps commented

2021-05-17 03:58:33 -05:00

(Migrated from github.com)

It's a little unconventional for a program to overwrite its own image.

Like I said - Architect of my own problem 8-)

I might see if I can somehow turn this into a separate bootloader, but I have more pressing project priorities at the moment so this might take a while.

> It's a little unconventional for a program to overwrite its own image. Like I said - Architect of my own problem 8-) I might see if I can somehow turn this into a separate bootloader, but I have more pressing project priorities at the moment so this might take a while.

smithps commented

2021-05-17 04:06:29 -05:00

(Migrated from github.com)

It's a little unconventional for a program to overwrite its own image.

Like I said - Architect of my own problem 8-)

I might see if I can somehow turn this into a separate bootloader, but I have more pressing project priorities at the moment so this might take a while.

I originally built it as part of my program image since that already had the code for dealing with the SD card filesystem, and I don't have much experience of playing around with linker files/relocating images.

Since there would be SD driver code in both my bootloader image and my program image, that would be rather wasteful of space. Is there a (simple) way I can put the SD card code in "my" bootloader, but then reference the code from within my application?

> > It's a little unconventional for a program to overwrite its own image. > > Like I said - Architect of my own problem 8-) > > I might see if I can somehow turn this into a separate bootloader, but I have more pressing project priorities at the moment so this might take a while. I originally built it as part of my program image since that already had the code for dealing with the SD card filesystem, and I don't have much experience of playing around with linker files/relocating images. Since there would be SD driver code in both my bootloader image and my program image, that would be rather wasteful of space. Is there a (simple) way I can put the SD card code in "my" bootloader, but then reference the code from within my application?

Ing-Dom commented

2021-07-27 09:59:49 -05:00

(Migrated from github.com)

I'm also very interested in a solution.
@smithps how did you solve the issue?

I'm also very interested in a solution. @smithps how did you solve the issue?

smithps commented

2021-07-27 11:07:17 -05:00

(Migrated from github.com)

I'm also very interested in a solution.
@smithps how did you solve the issue?

If you are asking about the __not_in_flash_func() bits - I just added that to the code I needed to and made it all work without disappearing into space.

In terms of the SD card functionality at the moment I have it working, but it relies on the SD card code from my app. That is it isn't a "standalone unit of code" (library) as such, but more of a suite of modules that end up being linked into my executable.

I modified my main() code;

to initialise the system hardware
Look for a file *.uf2 on the SD card
Compare its decoded contents with memory
If its different, then load it into the second half of flash
Copy that image over the top of my running code (making sure the routine doing the copying is in RAM)
Delete the UF2 file from the SD card
Reboot (This, for the life of me at the moment I can;t make work no matter how many different ways I try so at the moment it just hangs)
Once it hangs, turn it off then back on again.

I intend to improve this so that it also uses the LCD screen on my hardware to inform the user what's going on. Once this is complete I might try and turn it into something a bit more elegant, but as its quite hardware specific I'm not sure I can post it to the repository as an example.

> I'm also very interested in a solution. > @smithps how did you solve the issue? If you are asking about the __not_in_flash_func() bits - I just added that to the code I needed to and made it all work without disappearing into space. In terms of the SD card functionality at the moment I have it working, but it relies on the SD card code from my app. That is it isn't a "standalone unit of code" (library) as such, but more of a suite of modules that end up being linked into my executable. I modified my main() code; 1. to initialise the system hardware 2. Look for a file *.uf2 on the SD card 3. Compare its decoded contents with memory 4. If its different, then load it into the second half of flash 5. Copy that image over the top of my running code (making sure the routine doing the copying is in RAM) 6. Delete the UF2 file from the SD card 7. Reboot (This, for the life of me at the moment I can;t make work no matter how many different ways I try so at the moment it just hangs) 7. Once it hangs, turn it off then back on again. I intend to improve this so that it also uses the LCD screen on my hardware to inform the user what's going on. Once this is complete I might try and turn it into something a bit more elegant, but as its quite hardware specific I'm not sure I can post it to the repository as an example.

Ing-Dom commented

2021-07-27 11:20:12 -05:00

(Migrated from github.com)

I have a similar approach (no sd but field bus) and encountered (among others) the same problem as you did. you may have a look here when you interested, I shared code also: https://www.raspberrypi.org/forums/viewtopic.php?f=144&t=315267&p=1893934#p1893934

for reboot I choose the Watchdog, thats works great. have a look at the forum post of mine.

I have a similar approach (no sd but field bus) and encountered (among others) the same problem as you did. you may have a look here when you interested, I shared code also: https://www.raspberrypi.org/forums/viewtopic.php?f=144&t=315267&p=1893934#p1893934 for reboot I choose the Watchdog, thats works great. have a look at the forum post of mine.

kilograham commented

2021-10-04 16:28:27 -05:00

(Migrated from github.com)

note that __compiler_memory_barrier is now __force_inline so shouldn't be an issue - others will be dealt with separately

note that `__compiler_memory_barrier` is now __force_inline so shouldn't be an issue - others will be dealt with separately

kilograham commented

2021-10-12 09:17:16 -05:00

(Migrated from github.com)

merged into develop

Rows
Columns

__not_in_flash_func() requested around rom_func_lookup() and others. #410