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

RP235x support #66

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

RP235x support #66

wants to merge 5 commits into from

Conversation

CBJamo
Copy link

@CBJamo CBJamo commented Sep 23, 2024

The primary purpose of this pr is to support the new rx fifo modes on the rp235x, it also includes the new JMP_PIN source for WAIT and PINDIRS destination for MOV.

I'm not happy that I had to make a second lalrpop file, but I couldn't figure out how to make conditional compilation work.

Also open to bike shedding on the feature name.

@jannic
Copy link
Member

jannic commented Sep 27, 2024

I'm not happy that I had to make a second lalrpop file, but I couldn't figure out how to make conditional compilation work.

Would it be possible to use a single lalrpop file if it always understood the additions of RP235x? If every valid RP2040 pio program is also valid on the RP235x, that should work, and as I understand it this is how the additions were designed.

@CBJamo
Copy link
Author

CBJamo commented Sep 27, 2024

That's certainly possible, but then there wouldn't be compile time errors when trying to use a program with the new instructions on the 2040. Or at least there wouldn't be compile time errors originating in the lalrpop generated code like it would for any other invalid instruction.

Support for mov encodings to handle fifo get/put
Support for mov PINDIR dest
Support for wait JMPPIN source
@jannic
Copy link
Member

jannic commented Sep 28, 2024

That's certainly possible, but then there wouldn't be compile time errors when trying to use a program with the new instructions on the 2040. Or at least there wouldn't be compile time errors originating in the lalrpop generated code like it would for any other invalid instruction.

That's another advantage to do the parsing with rp235x support: There may be .pio files containing multiple programs, some rp2040 compatible and some with rp235x extensions. Such files should be usable on rp2040 as long as one only installs compatible programs. But with the current approach, the whole file would fail to parse.

Of course, now the question becomes at which point rp2040 compatibility should be checked. I did not yet think about it enough to have a clear picture.

One possibility would be that parse_file / parse_program only return programs compatible with the current microcontroller. That way, when using pio_proc, one would still get a compile error, albeit with a more generic and maybe a bit misleading error message like "program name not found in the provided file".

We could also somehow note the PIO version inside the program and then check it in the macro (which could then provide a better error message). Not sure if that can be sensibly done without a breaking change, which would in turn necessitate a new version of rp2040-hal.

@CBJamo
Copy link
Author

CBJamo commented Oct 1, 2024

Here is another attempt. I removed the rp2350 feature and added a version to the program struct, so pio-rs should be able to parse any program. I have mixed feelings about delegating handling the version to the HALs as I don't know how to make using an invalid program a compile time error there. This approach does make pio-rs much cleaner.

I also added support for the new Irq index modes. I think that was the only new feature (that belongs here in pio-rs) that was missing.

@CBJamo CBJamo changed the title RP235x new rx fifo modes support RP235x support Oct 1, 2024
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