Skip to content

Conversation

kilograham
Copy link
Contributor

fixes #2307

@kilograham kilograham added this to the 2.1.2 milestone Jul 9, 2025
@kilograham kilograham requested review from lurch and will-v-pi July 9, 2025 00:13
@lurch
Copy link
Contributor

lurch commented Jul 9, 2025

Is the change to amethyst.pio here intentional, or is that part of #2338 ?
This LGTM, but I won't approve because CI is failing 😉

@will-v-pi I wonder if it's worth having a CI job that builds a sample PIO file with each of the different output-formats that pioasm supports (ideally covering as many PIO keywords as possible?), and then comparing those newly-created outputs to "known good" output files?

Also, after this PR is merged I guess we'll need to update the pioasm-generated headers in the pico-examples repo.


header(out, "This file is autogenerated by pioasm; do not edit!");
std::stringstream header_string;
header_string << "This file is autogenerated by pioasm version " << PIOASM_VERSION_STRING << "; do not edit!";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be worth also including the filename (with any directory-parts removed) of the input PIO file? e.g.

// This file is autogenerated from ws2812.pio by pioasm version 2.1.2; do not edit!

@kilograham
Copy link
Contributor Author

Is the change to amethyst.pio here intentional, or is that part of #2338 ? This LGTM, but I won't approve because CI is failing 😉

@will-v-pi I wonder if it's worth having a CI job that builds a sample PIO file with each of the different output-formats that pioasm supports (ideally covering as many PIO keywords as possible?), and then comparing those newly-created outputs to "known good" output files?

Also, after this PR is merged I guess we'll need to update the pioasm-generated headers in the pico-examples repo.

it was unintentional, but then again the file isn't really used for anything - we should have a regression test that checks the output

Copy link
Contributor

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

LGTM - I will fix pico-sdk-tools to pass the new PIOASM_VERSION_STRING argument

will-v-pi added a commit to raspberrypi/pico-sdk-tools that referenced this pull request Jul 15, 2025
@kilograham kilograham merged commit d2cdf6c into develop Jul 15, 2025
11 checks passed
@kilograham kilograham deleted the pioasm-version branch July 15, 2025 20:09
will-v-pi added a commit to raspberrypi/pico-sdk-tools that referenced this pull request Jul 16, 2025
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