Arduino: correct the link mode guidance and pin the board core in CI - #21834
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21834
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit 3023c47 with merge base c461421 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Arduino library documentation and CI to reflect the Zephyr Uno Q core’s default “Dynamic” link mode behavior (which fails at compile/link time due to lack of effective --gc-sections under relocatable linking), and makes CI more reproducible by pinning the board core and key dependency versions.
Changes: (1) Correct and expand the README’s guidance/error table around link_mode, including updated size/memory measurements on core 0.90.0; (2) bump Arduino library metadata (version/author/maintainer) in library.properties; (3) harden and stabilize CI by pinning arduino:zephyr and Arduino_RouterBridge, adding retry/timeout tuning, and adding an informational Dynamic-mode compile to track the size gap.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| examples/arduino/README.md | Corrects Dynamic-vs-Static guidance, explains why Dynamic overflows, and refreshes memory/size figures for the current core. |
| examples/arduino/library.properties | Updates published library metadata and bumps version to 0.1.1. |
| .github/workflows/_test_arduino_library.yml | Pins Arduino core/dependency versions, adds download hardening, and records Dynamic-mode compile outcomes without gating CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
examples/arduino/README.md:257
- The last sentence reads as if both Static and Dynamic result in no serial output ("either way"), but only Dynamic fails at compile time; Static builds and can produce serial output. Consider clarifying that the lack of serial output is specific to the Dynamic-mode build failure (nothing gets flashed).
`Sketch too big; text section exceeds available space`. Same sketch, core
0.90.0: 507,876 bytes on Static, 787,508 on Dynamic. It fails at compile
time, so there is nothing to see on the serial port either way.
examples/arduino/library.properties:9
- PR description says
library.propertiesauthor should be updated toPyTorch Team <packages@pytorch.org>, but the file still listsMeta Platforms. This is a discrepancy that will affect published Library Manager metadata.
version=0.1.1
author=Meta Platforms
.github/workflows/_test_arduino_library.yml:78
- This job explicitly avoids using
HOMEbecause it may be unwritable in the container, butarduino-cli config initwrites the config underHOMEby default. As-is, CI may fail whenconfig initcan’t create/write its config directory. SetHOMEto a writable path (e.g.,${ARDUINO_CI_DIR}) before runningarduino-cli configcommands.
# The Zephyr core pulls a ~1 GB toolchain from a GitHub release, which
# outruns arduino-cli's default HTTP timeout often enough to matter, and
# `core install` has no retry of its own.
arduino-cli config init --overwrite
arduino-cli config set network.connection_timeout 600s
.github/workflows/_test_arduino_library.yml:94
- This script sources
.ci/scripts/utils.sh, which already defines aretry()function. Redefiningretry()here silently shadows the shared helper, which is easy to miss and can cause confusing behavior if later code expects the utils.sh semantics. Rename the local helper to avoid the collision.
retry() {
local n=1
until "$@"; do
if [ "${n}" -ge 3 ]; then
echo "::error::'$*' failed after ${n} attempts"
Two users hit the same wall within an hour of the library reaching Library Manager: install it, open an example, Verify, and get Sketch too big; text section exceeds available space The README described Dynamic link mode as producing "no serial output at all, so the board looks dead", which is wrong -- it fails at compile time, so a user searching for the error they actually saw found nothing. Corrected, and moved up into the error table with the real message. Why it happens, since the README had no explanation: Dynamic does a relocatable link (-r). --gc-sections is passed in both modes but can only work in a final link, where the linker has an entry point to trace reachability from. Under -r nothing can be proven unreachable, so every vendored operator survives, and because a loadable extension is loaded into RAM the retained code is charged against RAM too. AddModel on core 0.90.0: 507,876 bytes static, 787,508 dynamic. Recorded that the library cannot work around this. Trimming the vendored operator sources to only the registered set -- 15 instead of 172 -- moved the dynamic build 852 bytes. The bulk is the runtime, flatbuffers and CMSIS-NN. Memory figures were measured on core 0.55.2, which reported a 131,072-byte RAM ceiling; 0.90.0 reports 262,144, so they were not comparable to anything a current user sees. Re-measured all three examples on 0.90.0. The KeywordSpotting arena band is now scoped to the core it was measured on, with the upper bound marked untested on 0.90.0 -- 40 KB is confirmed, above that is not. CI pins arduino:zephyr@0.90.0 and Arduino_RouterBridge@0.4.3 rather than resolving to latest, so a failure means a real incompatibility instead of an upstream release landing under us. The core moved 0.55.2 -> 0.90.0 unannounced during this work. Adds the retry and network.connection_timeout hardening the ~1 GB toolchain download needs, since that toolchain is a ~1 GB GitHub release asset that outruns arduino-cli's default HTTP timeout. Bumps library.properties to version 0.1.1. Tracking of dynamic-mode sizes belongs in meta-pytorch/executorch-arduino, on a nightly, rather than here: it is a property of the published artifact, and three extra compiles per pull request touching runtime/ or kernels/portable/ is a poor trade for telemetry this repo cannot act on. Authored with assistance from Claude Code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/_test_arduino_library.yml:105
- The PR description says CI now performs an informational Dynamic (link_mode=dynamic) compile to track the size gap, but this workflow still only compiles sketches with link_mode=static. If Dynamic coverage is intended, add a non-gating Dynamic compile pass (expected to fail today) so the output is recorded and an unexpected success is flagged.
# link_mode=static is not optional. Dynamic is the board default and
# does a relocatable link, which makes --gc-sections inert: nothing
# unused is stripped and the build overflows flash.
FQBN="arduino:zephyr:unoq:link_mode=static"
for sketch in arduino_lib/ExecuTorch/examples/*/; do
echo "::group::compile $(basename "${sketch}")"
arduino-cli compile --fqbn "${FQBN}" \
--libraries arduino_lib "${sketch}"
echo "::endgroup::"
done
|
|
||
| name=ExecuTorch | ||
| version=0.1.0 | ||
| version=0.1.1 |
There was a problem hiding this comment.
Should we align these numbers with ET release?
Picks up pytorch/executorch#21834, which bumped library.properties to 0.1.1 and corrected the link mode and memory documentation upstream. Without the version bump the release cannot be published: the registry reads the version from library.properties and rejects a tag whose version is already indexed, and 0.1.0 is. library.properties and executorch_pin.txt now agree with the CHANGELOG entry for 0.1.1, which described the release before the generated content caught up. One runtime change comes with it. pytorch/executorch#20981, "Resolve Operator fallback to method allocator if temp alloc fails", landed between the two pins and is picked up in src/executorch/runtime/executor/method.cpp: resolve_operator now also falls back to the method allocator when the temp allocator is too small for the TensorMeta and dim-order arrays, not only when it is absent or empty. That is upstream content arriving through regeneration, which is how src/ is meant to move -- it is generated and never hand-edited here. arduino-lint reports no errors or warnings, and verify_models.py confirms all three example models match the regenerated runtime. Authored with assistance from Claude Code.
Picks up pytorch/executorch#21834, which bumped library.properties to 0.1.1 and corrected the link mode and memory documentation upstream. Without that bump the release cannot be published: the registry reads the version from library.properties and rejects a tag whose version is already indexed, and 0.1.0 is. library.properties and executorch_pin.txt now agree with the CHANGELOG entry for 0.1.1, which described the release before the generated content caught up. src/ is otherwise unchanged from 0.1.0 with one deliberate exception, recorded in extras/PROVENANCE.txt: method.cpp is held at the content from the previous pin. pytorch/executorch#20981 changed resolve_operator's temp-allocator fallback between the two pins. It is excluded here pending validation on the ET side, where its unittest-release/macos job was failing when it landed. That makes this tree deliberately not a byte-faithful copy of the pinned commit, which is why the deviation is written down rather than left for someone to discover by diffing. arduino-lint reports no errors or warnings, and verify_models.py confirms all three example models match. Authored with assistance from Claude Code.
Two users hit the same wall within an hour of the library reaching Library
Manager: install it, open an example, Verify, and get
Sketch too big; text section exceeds available space
The README described Dynamic link mode as producing "no serial output at all,
so the board looks dead", which is wrong -- it fails at compile time, so a
user searching for the error they actually saw found nothing. Corrected, and
moved up into the error table with the real message.
Why it happens, since the README had no explanation: Dynamic does a relocatable
link (-r). --gc-sections is passed in both modes but can only work in a final
link, where the linker has an entry point to trace reachability from. Under -r
nothing can be proven unreachable, so every vendored operator survives, and
because a loadable extension is loaded into RAM the retained code is charged
against RAM too. AddModel on core 0.90.0: 507,876 bytes static, 787,508 dynamic.
Recorded that the library cannot work around this. Trimming the vendored
operator sources to only the registered set -- 15 instead of 172 -- moved the
dynamic build 852 bytes. The bulk is the runtime, flatbuffers and CMSIS-NN.
Memory figures were measured on core 0.55.2, which reported a 131,072-byte RAM
ceiling; 0.90.0 reports 262,144, so they were not comparable to anything a
current user sees. Re-measured all three examples on 0.90.0. The KeywordSpotting
arena band is now scoped to the core it was measured on, with the upper bound
marked untested on 0.90.0 -- 40 KB is confirmed, above that is not.
CI pins arduino:zephyr@0.90.0 and Arduino_RouterBridge@0.4.3 rather than
resolving to latest, so a failure means a real incompatibility instead of an
upstream release landing under us. The core moved 0.55.2 -> 0.90.0 unannounced
during this work. Adds the retry and network.connection_timeout hardening the
~1 GB toolchain download needs, since that toolchain is a ~1 GB GitHub release
asset that outruns arduino-cli's default HTTP timeout.
Bumps library.properties to version 0.1.1.
Tracking of dynamic-mode sizes belongs in meta-pytorch/executorch-arduino, on a
nightly, rather than here: it is a property of the published artifact, and three
extra compiles per pull request touching runtime/ or kernels/portable/ is a poor
trade for telemetry this repo cannot act on.
Authored with assistance from Claude Code.