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

FRC target support #42063

Closed
wants to merge 3 commits into from
Closed

FRC target support #42063

wants to merge 3 commits into from

Conversation

connorworley
Copy link

This PR adds support for target platform used in the FIRST Robotics Competition.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@codyps
Copy link
Contributor

codyps commented May 17, 2017

Given that the control system has a habit of changing every few years, might make sense to expand the vendor to either include the year of introduction or the name of the platform.

@connorworley
Copy link
Author

connorworley commented May 17, 2017

Agreed, I've added the platform name and year of release.

Or do you mean changing the whole triple? Right now I'm just matching the triple that the NI-provided toolchain uses.

@aidanhs
Copy link
Member

aidanhs commented May 18, 2017

Thanks for the PR @connorworley! We'll check in regularly to make sure @arielb1 or another reviewer gets to this soon.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2017
@parched
Copy link
Contributor

parched commented May 18, 2017

I don't think rust is in the habit of adding vendored targets like this. Would adding a neutral armv7-unknown-linux-gnueabi target be sufficient for your needs?

@alexcrichton
Copy link
Member

Thanks for the PR @connorworley! As mentioned by @parched though we tend to add more generalized platforms to the compiler before branching into the specialized ones. We don't currently really have a policy one way or the other about this, it's mostly just gut feelings so far.

Out of curiosity is there a more general target that could be added? Or are custom target specs not sufficient for your use case maybe?

@connorworley
Copy link
Author

I could get away with using custom target specs, but the ultimate goal is for the high school students I work with to be able to easily deploy rust code for the roboRIO. In that regard, I think getting this target upstreamed would make it the easiest for them.

@arielb1 arielb1 assigned alexcrichton and unassigned arielb1 May 23, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 23, 2017

@alexcrichton are we going to be including this target? Should this be discussed in one of the teams?

@codyps
Copy link
Contributor

codyps commented May 24, 2017

Looks like this is just a specific cpu & features. Given that, it seems like naming it for the cpu selection & fpu options might be more appropriate (there isn't anything really FRC specific about the target, others with similar cpus could use it).

Is the need here just for better optimization compared to using one of the other arm-*-linux-gnueabi targets? More generally: what customizations here are actually required/desired and why are they required/desired?

@connorworley
Copy link
Author

So after some more testing I have a better idea of what's going on, and I think I can get away with just using arm-unknown-linux-gnueabi. If it turns out that any customization are actually necessary, I'll send in a new PR for a generic target supporting those customizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants