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

Maintainers for embedded targets #704

Closed
4 of 5 tasks
adamgreig opened this issue Oct 8, 2023 · 32 comments
Closed
4 of 5 tasks

Maintainers for embedded targets #704

adamgreig opened this issue Oct 8, 2023 · 32 comments

Comments

@adamgreig
Copy link
Member

adamgreig commented Oct 8, 2023

Currently many of the targets used for embedded development on Rust do not have official maintainers, because they were added before the current maintainer policy was adopted. There is currently work upstream to ensure all targets have maintainers, and it is likely that tier 2 targets that cannot find maintainers are eventually demoted.

rust-lang/rust#116004

After discussion in the weekly meetings, I'm proposing we nominate the @rust-embedded/cortex-m team to maintain the thumb*-none-eabi* targets. If the @rust-embedded/riscv team is interested we could also nominate them for the risc-v targets.

The main obligation that comes with being a target maintainer is being available to respond to occasional queries about the target or requests to test changes that the compiler team cannot; in general the idea is that the maintainers have an active interest in using the target and seeing it kept within Rust.

To be specific I'm suggesting we become the maintainer for: thumbv6m-none-eabi, thumbv7em-none-eabi, thumbv7em-none-eabihf, thumbv7m-none-eabi, thumbv8m.base-none-eabi, thumbv8m.main-none-eabi, and thumbv8m.main-none-eabihf.

I'm happy to put together the relevant PR if we approve:

I haven't spoken to the @rust-embedded/riscv team yet but if they're interested in the riscv32i-unknown-none-elf, riscv32imac-unknown-none-elf, riscv32imc-unknown-none-elf, riscv64gc-unknown-none-elf, riscv64imac-unknown-none-elf, or riscv32im-unknown-none-elf targets I'm happy to add them to the same PR (or they can make their own). Similarly if there are any other targets people in the WG think we could support.

@almindor
Copy link
Contributor

almindor commented Oct 8, 2023

I haven't spoken to the @rust-embedded/riscv team yet but if they're interested in the riscv32i-unknown-none-elf, riscv32imac-unknown-none-elf, riscv32imc-unknown-none-elf, riscv64gc-unknown-none-elf, riscv64imac-unknown-none-elf, or riscv32im-unknown-none-elf targets I'm happy to add them to the same PR (or they can make their own). Similarly if there are any other targets people in the WG think we could support.

I'm happy to help with riscv32imac as that's what I have, I cannot do practical things with the others I'm afraid.

@cr1901
Copy link

cr1901 commented Oct 8, 2023

Apparently Tier 3 targets need a maintainer too. I'm fine w/ either the msp430 team or myself being a maintainer (cc: @YuhanLiin, would you be open to maintaining it too?). I already kinda maintain it, anyway.

@newAM
Copy link
Member

newAM commented Oct 8, 2023

I'm onboard to help maintain these targets! I don't know as much as I would like to about the thumv8m ISA though 😅

@thejpster
Copy link
Contributor

So this is a good backstop, but honestly I'd much rather Arm was listed as the maintainer, as they are for aarch64-unknown-linux-gnu. I understand that they will already help fix issues if we tag the right people, but there's some discussion about whether they're happy to have their name actually listed.

@thejpster
Copy link
Contributor

Equally it would be nice if the RISC-V targets were maintained by the RISC-V Foundation, or if they sponsored someone to maintain them.

@therealprof
Copy link
Contributor

So this is a good backstop, but honestly I'd much rather Arm was listed as the maintainer, as they are for aarch64-unknown-linux-gnu. I understand that they will already help fix issues if we tag the right people, but there's some discussion about whether they're happy to have their name actually listed.

I agree that having the official makers of the stuff onboard to be co-maintainers would be better. In any case we additionally should have rewg people on board rather than outsourcing this completely without any possibility of collaboration...

@thejpster
Copy link
Contributor

That's a fair point. We should be co-maintainers.

@thejpster
Copy link
Contributor

Let's get the cortex-m team added now, and I'll continue to talk to Arm about this. They could be co-maintainers, or they could just have someone join the cortex-m team. Either would work.

@romancardenas
Copy link
Contributor

I haven't spoken to the @rust-embedded/riscv team yet but if they're interested in the riscv32i-unknown-none-elf, riscv32imac-unknown-none-elf, riscv32imc-unknown-none-elf, riscv64gc-unknown-none-elf, riscv64imac-unknown-none-elf, or riscv32im-unknown-none-elf targets I'm happy to add them to the same PR (or they can make their own). Similarly if there are any other targets people in the WG think we could support.

I'm happy to help with riscv32imac as that's what I have, I cannot do practical things with the others I'm afraid.

Same here, I can help with RISC-V 32 targets. I currently do not have any RISC-V 64 board, but I plan to eventually work with those targets as well.

@adamgreig
Copy link
Member Author

adamgreig commented Oct 8, 2023

Thanks for the responses everyone. I've started to put together a branch with the relevant details.

Please could you check the relevant files, let me know if anything is missing or wrong, if there's any more details you think should be present, or any other links or docs:

  • MSP430
  • RV32
    • I've combined i, imc, and imac into one file, as they're all tier 2 targets with no existing details
    • I've left im alone because it's somehow tier 3 and does have a nominal maintainer, though it might make more sense to see if we can have it promoted to tier 2 and then its platform file merged with this one
    • The "building rust programs" and "cross-compilation toolchains and C code" sections are missing, any ideas here? The template has a little guidance of what to put in each section.
  • thumbv6
  • thumbv7
  • thumbv8
    • I merged baseline and mainline as I expect the details will be the same for them and we've merged all the v7 targets together too.

Feel free to leave comments here or open a PR against my branch, once we're happy with it I'll submit it as a PR to rust-lang/rust#116004

@cr1901
Copy link

cr1901 commented Oct 9, 2023

This target supports interlinking with C code compiled using the TI MSP430 GCC compiler.

Yes this is correct, but I would put a caveat that cross-language LTO w/ the GCC compiler almost certainly doesn't work (b/c LLVM vs GCC's IR). I would also put a note that nightly is required for the forseeable future until at least rust-lang/rfcs#3246 and by extension rust-lang/rust#38487 are resolved. Because we're stuck on nightly, I have not felt an impetus to get rid of the build-std=core invocation (and honestly, I kinda hope at some point build-std is just stabilized and we don't need to provide libcore).

@jonathanpallant
Copy link
Contributor

thumbv8m.main-none-eabi points to thumbv8m.main-none-eabihf

but

thumbv7em-none-eabihf points to thumbv7m-none-eabi

We should probably have the 'lowest' target as the one that gets the file and then have the others point at it? Doesn't make a big difference though.

@jonathanpallant
Copy link
Contributor

The -eabihf target enables the hardware FPU and uses the hard-float ABI.

I would add:

If you want to use the FPU with the soft-float ABI, you can use the -eabi' target, but add extra RUSTFLAGSlike-mcpu=+vfp` (I made that up).

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Oct 9, 2023

The thumbv8m file says:

This target supports C code. Use the thumbv6m-none-eabi triple in LLVM. If interlinking with C or C++, you may need to use arm-none-eabi-gcc as a linker instead of the built-in LLD.

That's probably the wrong LLVM target? Also s/the built-in LLD/the default rust-lld linker/.

@jonathanpallant
Copy link
Contributor

Maybe in the thumbv6m-none-eabi target file we could point them at atomic-polyfill?

Also I think my notes above apply to the thumbv7m target file too; but I really liked the Floating Point section so maybe copy that to the thumbv8 file?

@adamgreig
Copy link
Member Author

Thanks @cr1901, @jonathanpallant, I've made those changes and pushed, except:

If you want to use the FPU with the soft-float ABI, you can use the -eabi' target, but add extra RUSTFLAGSlike-mcpu=+vfp` (I made that up).

Happy to add this but not sure what the right incantation is and don't have time to test it tonight, does anyone know?

@thejpster
Copy link
Contributor

I guess we can add it later.

@adamgreig
Copy link
Member Author

Thinking about it more, I think it would be more involved than just the config flag - you'd cause rust to generate FPU instructions, but cortex-m-rt uses the "hf" in the target to know to enable the FPU at boot, so you'd also need to manually enable the FPU. Perhaps advanced enough to leave out of the target support for the time being.

@jonathanpallant
Copy link
Contributor

Yeah but that's just a method in cortex-m to poke CP15, right? And if you want to use an RTOS that doesn't use HF (because stacking those registers is expensive) you will want this.

@YuhanLiin
Copy link
Contributor

@cr1901 I've fallen off embedded Rust lately, so I'm not really the right person for maintenance duty.

@adamgreig
Copy link
Member Author

@romancardenas, @almindor, would you mind checking the proposed rv32 file? In particular

  • I've combined i, imc, and imac into one file, as they're all tier 2 targets with no existing details
  • I've left im alone because it's somehow tier 3 and does have a nominal maintainer, though it might make more sense to see if we can have it promoted to tier 2 and then its platform file merged with this one
  • The "building rust programs" and "cross-compilation toolchains and C code" sections are missing, any ideas here? The template has a little guidance of what to put in each section.

@MabezDev
Copy link
Member

@romancardenas, @almindor, would you mind checking the proposed rv32 file? In particular

  • I've combined i, imc, and imac into one file, as they're all tier 2 targets with no existing details
  • I've left im alone because it's somehow tier 3 and does have a nominal maintainer, though it might make more sense to see if we can have it promoted to tier 2 and then its platform file merged with this one
  • The "building rust programs" and "cross-compilation toolchains and C code" sections are missing, any ideas here? The template has a little guidance of what to put in each section.

I agree with merging the targets into one file. I also think that we should pursue making the im target tier 2 if possible, I don't see a reason why that would be rejected. I think we can just copy the no_std arm target sections for the cross-compilation and building rust program sections.

FYI the riscv-rust-quickstart link 404s, I guess the link should be https://github.com/riscv-rust/riscv-rust-quickstart, but this seems to be a quickstart for the discontinued hifive boards. It might make sense to update this to something more modern (and purchasable) before adding the link here.

@adamgreig
Copy link
Member Author

I also think that we should pursue making the im target tier 2 if possible, I don't see a reason why that would be rejected. I think we can just copy the no_std arm target sections for the cross-compilation and building rust program sections.

I like the idea but don't know anything about practically doing it in rust-lang/rust, do you think that should wait until after these target changes are merged (assuming it needs some sort of signoff or changes to what gets distributed via rustup etc)?

@almindor
Copy link
Contributor

FYI the riscv-rust-quickstart link 404s, I guess the link should be https://github.com/riscv-rust/riscv-rust-quickstart, but this seems to be a quickstart for the discontinued hifive boards. It might make sense to update this to something more modern (and purchasable) before adding the link here.

The red-v clone is still available though I suspect it'll be out of circulation soon as well. The quickstart was started when hifive1 was essentially the only board around, situation is quite different now but ironically I don't have any of the newer ones :D

@almindor
Copy link
Contributor

@romancardenas, @almindor, would you mind checking the proposed rv32 file? In particular

  • I've combined i, imc, and imac into one file, as they're all tier 2 targets with no existing details
  • I've left im alone because it's somehow tier 3 and does have a nominal maintainer, though it might make more sense to see if we can have it promoted to tier 2 and then its platform file merged with this one
  • The "building rust programs" and "cross-compilation toolchains and C code" sections are missing, any ideas here? The template has a little guidance of what to put in each section.

LGTM apart from the quickstart link as mentioned by @MabezDev

@adamgreig
Copy link
Member Author

Thanks! I've removed the quickstart link for now.

The final thing is the missing "building rust programs" and "cross-compilation toolchains and C code" for riscv32, any ideas there?

@romancardenas
Copy link
Contributor

Looks good to me!!

I'd update the riscv-rust-quickstart repo to align it with a RED-V clone for now. I have a couple of RED-V boards, so I can take a look. But we definitely need a more generic and modern template.

Regarding cross-compilation toolchains and C code, I haven't played around with it, but I guess that we could add something like:

This target supports C code. If interlinking with C or C++, you may need to use riscv64-unknown-elf-gcc as a linker instead of rust-lld.

@MabezDev
Copy link
Member

I like the idea but don't know anything about practically doing it in rust-lang/rust, do you think that should wait until after these target changes are merged (assuming it needs some sort of signoff or changes to what gets distributed via rustup etc)?

I can take care of that as I'll be adding the imafc target soon for the ESP32-P4.

@cr1901
Copy link

cr1901 commented Oct 16, 2023

@cr1901 I've fallen off embedded Rust lately, so I'm not really the right person for maintenance duty.

@YuhanLiin Okay, no problem. I can take care of things. I've also been on a bit of a break from writing Rust, embedded or otherwise. My priority right now is to just ensure that MSP430 doesn't break e.g. AT2XT until I have the bandwidth to make a bunch of small improvements in a large batch (such as "it's now agreed that volatile references need to be replaced with pointer accesses, even though in the short term nothing is going to break").

@adamgreig
Copy link
Member Author

adamgreig commented Oct 17, 2023

@romancardenas

I'd update the riscv-rust-quickstart repo to align it with a RED-V clone for now. I have a couple of RED-V boards, so I can take a look. But we definitely need a more generic and modern template.

OK, I've re-instated the (correct) link, I guess at least it's something and hopefully it can be updated to be more useful.

Regarding cross-compilation toolchains and C code, I haven't played around with it, but I guess that we could add something like

Thanks, added.

@MabezDev

I can take care of that as I'll be adding the imafc target soon for the ESP32-P4.

Brilliant, thanks! I'll leave it as-is for this PR then, and you can merge the rv32im target to use the same platform support file at the same time?


I've pushed the latest updates and I don't think anything is missing now, so I'll bring it up at the weekly meeting tomorrow and if there are no further comments after that I'll open the PR upstream.

@jannic
Copy link
Member

jannic commented Aug 13, 2024

As far as I can tell the goal of this ticket has been reached and the teams are listed as maintainers of their architectures in https://github.com/rust-lang/rust/tree/master/src/doc/rustc/src/platform-support/
Can we close this ticket?

@adamgreig
Copy link
Member Author

Thanks, yep! It's not the route we first planned but you're right, all the goals have been reached so this can be closed.

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

No branches or pull requests