Skip to content

Fix for PANIC if using sntp with pico-w (#1588) #1598

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gpummer
Copy link

@gpummer gpummer commented Mar 23, 2025

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

See #1588.

I compiled (and run) this change with src/platforms/rp2 for pico and pico_w. I really would like to test this with pico2_w, but this platform seems not available at the moment, because _deps/pico_sdk-src is a little bit older or not available right now.

I learned about lwip and LWIP_NUM_SYS_TIMEOUT_INTERNAL and how it is calculated. AtomVM is a little bit different in this regard, because sntp is optional for the VM user. I hope it is acceptable to waste one lwip memory slot if sntp is not used at all.

If needed, I think I can test this change in the release-0.6 branch. Tried this already, but had some troubles with a not set doxygen-found variable in the build script.

This is my first pull request, so please be kind ;) All infos are appreciated and I will try to follow them.

@petermm
Copy link
Contributor

petermm commented Mar 23, 2025

Great find/work!

If you can squash your commits, and use --signoff option for the commit.. that should take care of minimal requirements.

I don't have a pico, so someone else will feedback on actual content of PR, LGTM.

@gpummer
Copy link
Author

gpummer commented Mar 23, 2025

Squash and --signoff done. I read that in the contribution.md but missed that. Sorry!

@pguyot
Copy link
Collaborator

pguyot commented Mar 29, 2025

@petermm
Copy link
Contributor

petermm commented Mar 29, 2025

should we rebase on release-0.6 branch?

@bettio
Copy link
Collaborator

bettio commented Mar 29, 2025

should we rebase on release-0.6 branch?

The rule is: if it something that can be fixed in release-0.6, yes. So I think so.
Let's also add an entry to the changelog.

@petermm
Copy link
Contributor

petermm commented Apr 25, 2025

@gpummer if you have the chance could you rebase on release-0.6 and add a line to the changelog?

@gpummer
Copy link
Author

gpummer commented Apr 26, 2025

@petermm Added to my list, will do it this weekend! (just a little bit anxious about changelog, but I will try)

@gpummer
Copy link
Author

gpummer commented Apr 27, 2025

Just to be safe and doing no harm...
@petermm I learned a lot today about github, upstream branches in the fork, git rebase and magic (src/platforms/rp2 -> rp2040). Fortunately I tested this new build on my Pico W. Unfortunately the VM starts with the correct Version (0.6.6...), connects to the WLAN but then stops without any message (in a simple timer:sleep/1). It doesn't matter if sntp is on or off. I have to investigate if this has something to do with the fix (which is working with 0.7) or the build itself. So please, give me a day or two.

@bettio
Copy link
Collaborator

bettio commented Apr 29, 2025

I suggest you to make sure you didn't pull any newer (unrelated change) by mistake.
If you rebase your PR on top of release-0.6 you should have (when doing git log) [1] and just your commit on top of that (you can also check this with git diff release-0.6, you should notice just your change)`.

Maybe here @pguyot might have some advices on pico.

[1]:

commit 5eb7cf85d2bcfe8d4ac7004b816cd70b9ee04f52 (origin/release-0.6, release-0.6)
Merge: 54cf1159 9f299e7e
Author: Davide Bettio <[email protected]>
Date:   Thu Apr 24 20:52:45 2025 +0200

    Merge pull request #1635 from pguyot/w16/fix-unlink-protocol

    Implement link protocol with unlink id and acknowledgement

    These changes are made under both the "Apache 2.0" and the "GNU Lesser General
    Public License 2.1 or later" license terms (dual license).

    SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

@gpummer
Copy link
Author

gpummer commented May 1, 2025

Many thanks @bettio ! I double checked this. My "git log" and "git diff release-0.6" is as you wrote.

@gpummer
Copy link
Author

gpummer commented May 1, 2025

Update: I think the cause is the build for "-DPICO_BOARD=pico_w". I got no stable VM if I build for pico_w. The application is always hanging in a timer:sleep/1 after startup. Sometimes 2 or 3 timer:sleep/1 more, but then it stops. It doesn't matter if I use network, or build with "release-0.6" branch or my rebased branch. Maybe timer:sleep/1 is a false observation, I don't know for sure at the moment. I have also tried to connect my pico-w directly to a USB power supply unit and tried different cables.

If I remove "-DPICO_BOARD=pico_w" and build for "pico" the built VM is working stable. My simple "blink to different leds in different rhythm" erl program is running since 30 minutes now. (At last some soldering.)

So, I think my "build environment" (Raspberry PI 4 with bookworm) is ok, it has something to do with the differences between pico and pico_w. I will try to find differences in the "makefiles". I appreciate any hints :)

@pguyot
Copy link
Collaborator

pguyot commented May 1, 2025

Are you using the official pico sdk somehow or is the environment clean and AtomVM picks up my fork? What you describe is similar to the issues I had when I tried to upgrade Pico SDK to be able to support Pico 2W.

I confirm that picow_blink.uf2 works on a Pico-W and -DPICO_BOARD=pico_w with your branch (gpummer/issue-1588, i.e. based on main), as well as with release-0.6 (5eb7cf8) with your commit cherry-picked.

You may have PICO_SDK_PATH in your environment that is picked up by pico_sdk_import.cmake. I believe we should edit this file to avoid using PICO_SDK_PATH to prevent such issues.

@gpummer
Copy link
Author

gpummer commented May 1, 2025

I think the environment is clean. If I don't unset PICO_SDK_PATH there is a compile error. But to be sure I have unset all 4 PICO_* environment variables and did a clean build (rm -Rf build; mkdir build ...).

Very cool to hear that this works in your environment. Sorry for the additional trouble. It was meant to help you, not to add more tasks :(

gpummer@jupiter:~/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build $ cmake .. -G Ninja -DPICO_BOARD=pico_w
Downloading Raspberry Pi Pico SDK
...
PICO target board is pico_w.
Using CMake board configuration from /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/src/boards/pico_w.cmake
Using board configuration from /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/src/boards/include/boards/pico_w.h
-- Found Python3: /usr/bin/python3 (found version "3.11.2") found components: Interpreter 
TinyUSB available at /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/lib/tinyusb/src/portable/raspberrypi/rp2040; enabling build support for USB.
BTstack available at /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/lib/btstack
cyw43-driver available at /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/lib/cyw43-driver
Pico W Bluetooth build support available.
lwIP available at /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/lib/lwip
Pico W Wi-Fi build support available.
mbedtls available at /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/_deps/pico_sdk-src/lib/mbedtls
gpummer@jupiter:~/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build $ ninja
[19/1113] Performing configure step for 'PioasmBuild'
loading initial cache file /home/gpummer/picospace/github-atomspace/AtomVM/src/platforms/rp2040/build/pico-sdk/src/rp2_common/pico_cyw43_driver/pioasm/tmp/PioasmBuild-cache-Release.cmake
-- The CXX compiler identification is GNU 12.2.0
-- Detecting CXX compiler ABI info
...

I will try the example you mentioned with my built VM and investigate this further.

@gpummer
Copy link
Author

gpummer commented May 4, 2025

Update: If I use my built VM and try the mentioned picow-blink example this works. Also the erlang blinky example works with an external LED.

But I was not successful with a source based on the blinky example having two process/2, spawn/4 calls. Using this generated uf2 on a pico device works for multiple hours - VM built on the same raspberry, for target pico. So I am a little bit clueless at the moment. I am on a business trip next week.

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.

4 participants