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

R2000ic 210l support #69

Conversation

demorise
Copy link
Contributor

@demorise demorise commented Mar 29, 2022

@gavanderhoorn
fanuc_2000ic_210l

Test results:

tf_echo
teach_pendant_tf

@gavanderhoorn
Copy link
Member

Thanks for the PR.

The commit history seems a bit strange though: the second commit seems to delete all files related to the CRX-10iA/L, which were added by the first commit.

Was that what you intended to do?

@demorise
Copy link
Contributor Author

Thanks for the PR.

The commit history seems a bit strange though: the second commit seems to delete all files related to the CRX-10iA/L, which were added by the first commit.

Was that what you intended to do?

Yes, I was trying to separate the two PRs since the CRX-10iA/L PR was yet to be merged. If you prefer that I include both commits in one PR, I can do that.

@gavanderhoorn
Copy link
Member

The easiest way to go about this would be if the /210L branch would not have the crx10ial_support branch as "parent", but it would be a branch off of melodic-devel. Seeing as the two support packages have nothing shared between them, that would have been possible.

In your current PR, ac123a8 physically removes the CRX-10iA/L files which were added in 0fcfd93.

If I were to merge this PR, in its current state, I'd effectively reverse #67.

That's not what we want.

Seeing as I've not reviewed this PR, and it's only a single commit right now, I'd suggest creating a new branch (off of melodic-devel), adding the new package, committing it, and then submitting a PR from that new branch.

Alternatively, you could re-add the CRX-10iA/L files to your local clone, update ac123a8 to include them and then force-push. That may be more involved though.

@demorise
Copy link
Contributor Author

Closing because it has been replaced by #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants