Conversation
github-actions
bot
added
needs-review
punkle
force-pushed
the
add-size-change-report
branch
from
9216ede to
bbbdaf0
Compare
|
I am not sure how I can test this.. The PR job seems to be using the develop branch, maybe I can test this after its merged which means we may need to be on hand to revert if it goes wrong. |
|
The manifests are used by the webflasher, I think. You could test this in your own repo by tweaking the actions and running it against dummy PRs to your own develop branch? I really want this, btw! |
… add-size-change-report
|
@NomDeTom thanks. working on trying to test it now 🤞 |
meshtastic#9934) * fix: MQTT settings silently fail to persist when broker is unreachable isValidConfig() was testing broker connectivity via connectPubSub() as part of config validation. When the broker was unreachable (network not ready, DNS failure, server down), the function returned false, causing AdminModule to skip saving settings entirely — silently. This removes the connectivity test from isValidConfig(), which now only validates configuration correctness (TLS support, default server port). Connectivity is handled by the MQTT module's existing reconnect loop. Fixes meshtastic#9107 * Add client warning notification when MQTT broker is unreachable Per maintainer feedback: instead of silently saving when the broker can't be reached, send a WARNING notification to the client saying "MQTT settings saved, but could not reach the MQTT server." Settings still always persist regardless of connectivity — the core fix from the previous commit is preserved. The notification is purely advisory so users know to double-check their server address and credentials if the connection test fails. When the network is not available at all, the connectivity check is skipped entirely with a log message. * Address Copilot review feedback - Fix warning message wording: "Settings will be saved" instead of "Settings saved" (notification fires before AdminModule persists) - Add null check on clientNotificationPool.allocZeroed() to prevent crash if pool is exhausted (matches AdminModule::sendWarning pattern) - Fix test comments to accurately describe conditional connectivity check behavior and IS_RUNNING_TESTS compile-out * Remove connectivity check from isValidConfig entirely Reverts the advisory connectivity check added in the previous commit. While the intent was to warn users about unreachable brokers, connectPubSub() mutates the isConnected state of the running MQTT module and performs synchronous network operations that can block the config-save path. The cleanest approach: isValidConfig() validates config correctness only (TLS support, default server port). The MQTT reconnect loop handles connectivity after settings are persisted and the device reboots. If the broker is unreachable, the user will see it in the MQTT connection status — no special notification needed. This returns to the simpler design from the first commit, which was tested on hardware and confirmed working. * Use lightweight TCP check instead of connectPubSub for validation Per maintainer feedback: users need connectivity feedback, but connectPubSub() mutates the module's isConnected state. This uses a standalone MQTTClient TCP connection test that: - Checks if the server IP/port is reachable - Sends a WARNING notification if unreachable - Does NOT establish an MQTT session or mutate any module state - Does NOT block saving — isValidConfig always returns true The TCP test client is created locally, used, and destroyed within the function scope. No side effects on the running MQTT module. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
…missing hop in traceroute (meshtastic#9945) * Fix issue 9792, decode packet for TR test * Fix 9792: Assure packet id decoded for TR test * Potential fix for pull request finding Log improvement for failure to decode packet. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * trunk fmt --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Tom Fifield <tom@tomfifield.net>
…tastic#9953) The touch-to-backlight feature was gated behind TTGO_T_ECHO_PLUS, but the regular T-Echo has the same backlight pin (PIN_EINK_EN, P1.11). This changes the guard to use PIN_EINK_EN only, so any device with an e-ink backlight pin gets the feature. Fixes meshtastic#7630 Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
* add heltec_mesh_node_t096 board. * Fixed the GPS reset pin comments. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Added compiles if NUM_PA_POINTS is not defined. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Correct the pin description. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Specify the version of the dependency library TFT_eSPI. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Adding fields missing from the .ini file. * Modify the screen SPI frequency to 40 MHz. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
…shtastic#9971) * Fixes double space OLEDDisplayFontsRU.cpp * Fixes double space OLEDDisplayFontsUA.cpp --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds automation to collect firmware binary sizes from build manifests, compare them against recent baselines, and publish a Markdown size-change report back to the PR.
Changes:
- Add
bin/collect_sizes.pyto aggregate.mt.jsonmanifest data into a{target: bytes}JSON report. - Add
bin/size_report.pyto compare current sizes vs one or more baselines and generate a Markdown table/summary. - Extend CI workflow to upload size artifacts and (on
pull_request_target) generate and post/update a PR size report.
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
bin/size_report.py |
Generates Markdown (summary + table + collapsible section) for size deltas vs baselines. |
bin/collect_sizes.py |
Extracts firmware .bin size per PlatformIO target from .mt.json manifests into a single JSON. |
.github/workflows/main_matrix.yml |
New firmware-size-report job to collect/upload sizes, fetch develop/master baselines, and publish the report to PRs. |
| RUN_ID=$(gh run list -R "${{ github.repository }}" \ | ||
| --workflow CI --branch develop --status success \ | ||
| --limit 1 --json databaseId --jq '.[0].databaseId') | ||
| if [ -n "$RUN_ID" ]; then |
There was a problem hiding this comment.
gh run list ... --jq '.[0].databaseId' will output null (a non-empty string) when there are no matching runs. The subsequent if [ -n "$RUN_ID" ] check will treat null as valid and the download logic will fail. Consider changing the jq to '.[0].databaseId // empty' (or explicitly checking RUN_ID != "null").
.github/workflows/main_matrix.yml
Outdated
| run: | | ||
| RUN_ID=$(gh run list -R "${{ github.repository }}" \ | ||
| --workflow CI --branch master --status success \ | ||
| --limit 1 --json databaseId --jq '.[0].databaseId') |
There was a problem hiding this comment.
Same issue as the develop baseline: --jq '.[0].databaseId' can produce null, which passes the -n check and then breaks the artifact download path. Use // empty in the jq filter or guard against the literal null value.
| --limit 1 --json databaseId --jq '.[0].databaseId') | |
| --limit 1 --json databaseId --jq '.[0].databaseId // empty') |
| - name: Post or update PR comment | ||
| if: github.event_name == 'pull_request_target' && steps.report.outputs.has_report == 'true' | ||
| uses: actions/github-script@v8 |
There was a problem hiding this comment.
PR description says this should be added to PR descriptions, but this workflow posts/updates a PR comment instead. If the intent is to update the PR body, use the Pull Requests API (pulls.update) to update pull_request.body (and still keep the marker to make it idempotent).
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| actions: read |
There was a problem hiding this comment.
This job runs on pull_request_target and checks out/runs code from the PR branch while granting pull-requests: write (and actions: read). That combination allows untrusted PR code to use the elevated GITHUB_TOKEN to modify PR metadata/comments (and potentially exfiltrate token-scoped data). Consider generating the report using code from the base branch (checkout github.event.pull_request.base.sha or default branch) and only using PR artifacts as inputs, or split into two jobs so the commenting job never executes PR-provided code.
| - uses: actions/checkout@v6 | ||
| if: github.event_name == 'pull_request_target' | ||
| with: | ||
| filter: blob:none # means we download all the git history but none of the commit (except ones with checkout like the head) | ||
| fetch-depth: 0 | ||
| - name: Download the current manifests | ||
| ref: ${{github.event.pull_request.head.ref}} | ||
| repository: ${{github.event.pull_request.head.repo.full_name}} | ||
|
|
There was a problem hiding this comment.
This checkout uses github.event.pull_request.* fields unconditionally. On push events those fields are absent, and on pull_request_target it checks out untrusted PR code. If you keep this job running on both push (to create baselines) and pull_request_target (to comment), it’s safer/cleaner to conditionally set ref/repository only for PR events, and otherwise do a normal checkout of the current repo/commit (or the PR base SHA for the reporting logic).
…add-size-change-report
|
I messed up a merge.. I merged the into master instead of develop. Im temped to start again. |
Add a size change report to pr descriptions.
🤝 Attestations