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

don't go through runtime initializers when there is no OTA command #2697

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

afflux
Copy link
Contributor

@afflux afflux commented Dec 17, 2024

Leave that to the main app instead, so we don't reset peripherals twice.

See discussion here: #2686

@afflux
Copy link
Contributor Author

afflux commented Dec 17, 2024

I've verified that my problem is solved with this change, but I haven't given OTA much testing at this point.

Leave that to the main app instead, so we don't reset peripherals twice.
@afflux afflux force-pushed the ota-fix-runtimeinit branch from d522866 to ecea8bd Compare December 17, 2024 20:26
@afflux
Copy link
Contributor Author

afflux commented Dec 17, 2024

OTA works on my Pico W

@afflux afflux marked this pull request as ready for review December 17, 2024 20:31
Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to rebuild all 3 OTAs and add them to the PR, please. The IDE doesn't even look at the /ota directory or the source files there. they are included as an opaque blob since they have duplicated symbols from the main app.

@afflux
Copy link
Contributor Author

afflux commented Dec 18, 2024

Done. Note that the OTA build currently tries to copy riscv header files somewhere and fails:

cp: cannot create regular file '../../include/rp2350-riscv/pico_base/pico/.': No such file or directory

Let me know if you want me to clean that up in this PR as well

@afflux
Copy link
Contributor Author

afflux commented Dec 18, 2024

One thing to note: I'm reordering the runtime_init priorities a little, because reading the LFS to check if there is an OTA available requires working memset/memcmp/memcpy, which is wired to the bootrom functions and therefore requires __aeabi_mem_init to have run. I have tested this on rp2040, but I don't know if rp2350{,-riscv} needs other runtime_init functions.

https://github.com/raspberrypi/pico-sdk/blob/2.1.0/src/rp2_common/pico_runtime_init/include/pico/runtime_init.h

And now that I think of it, it might make sense to also __aeabi_{bits,float,double}_init to run before check_ota(), just in case one of those functions end up being called by LittleFS in some code path I haven't seen yet.

@earlephilhower
Copy link
Owner

cp: cannot create regular file '../../include/rp2350-riscv/pico_base/pico/.': No such file or directory
Let me know if you want me to clean that up in this PR as well

Could you, please? I have it in my own tree but I forgot to git add include/rp2350-riscv/pico_base/pico/ota_command.h

@afflux
Copy link
Contributor Author

afflux commented Dec 18, 2024

There doesn't appear to be anything referencing include/rp2350-riscv, instead the riscv build will also use the include/rp2350 headers, so I would've just disabled the copy for riscv:

-iwithprefixbefore/include/rp2350
-iwithprefixbefore/include/rp2350/pico_base

@earlephilhower
Copy link
Owner

earlephilhower commented Dec 18, 2024

The problem is then you have to special case the R5 build. We still need it copied over in the rp2350 and rp2040 one. It's doable, but maybe not so clean in cmake?

-edit- You're right, it shouldn't be copied over in the R5 case and it's better not to create a directory and put files into it when those files won't actually ever be used. If you can make it so just the rp2040 and rp2350 builds to it that would be very much appreciated and much cleaner SW engineering!

@earlephilhower
Copy link
Owner

A quick run of OTAFromFile on the Pico2 using both ARM and RISC-V modes seems to still work, so this looks good!

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, no effect on Pico2 ARM or RISC-V (there's no mem-ops there, ROM doesn't include much at all and is very slow).

Float/double aren't in LittleFS so no worries about including that (softfp would blow up the 12KB we reserve for OTA code so this seems safe) but they could be moved up.

The bit operations probably will be used, good point. I'm not sure the filesystem needs to do CLZs and POPCOUNT, but we have a GZIP decompressor that can run under OTA which in my experience is going to want those special builtins.

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good now. Tested Pico2 and Pico and GZip compressed OTA, no issues seen.

@earlephilhower earlephilhower merged commit 4d03edc into earlephilhower:master Dec 19, 2024
25 of 26 checks passed
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

Successfully merging this pull request may close these issues.

2 participants