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

Cleaned up degrees to radians calculation #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cleaned up degrees to radians calculation #5

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 5, 2018

Created a degrees to radians function and removed a few unnecessary conversions
Constants are still in degrees but are converted

Created a degrees to radians function  and removed a few unnecessary conversions
Constants are still in degrees but are converted
@XRobots
Copy link
Owner

XRobots commented Jun 6, 2018

Thanks for that, although I'm not using Git in the 'proper way' for the early videos because if someone watches the video and then looks on Git I want them to see the same code. Most of my normal viewers aren't aware of versioning etc and I want to make the entry point to understanding really easy.

For this reason, any changes in part 2 of the video will be in a separate folder called 'part 2'.

Also this is only one piece of the jigsaw for solving the kinematics, which probably needs to be it's own function so it can be used for each leg. Eventually I'll have a 'working version' that I can merge contributions into on an ongoing basis.

@jantje
Copy link

jantje commented Jun 6, 2018

If you do not want to go for releases; wouldn't it be easier to have a "master folder" to work on and copy it to partX folder at release time?

@XRobots
Copy link
Owner

XRobots commented Jun 6, 2018

I'll go for releases in the end, but for now it's more like 'code samples', so there may be completely new bits of test code that relate to each video, or it'll all get scrapped entirely.

Copy link

@adamgreen adamgreen left a comment

Choose a reason for hiding this comment

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

In general I find that this change does make the code a bit easier to read.

Based on James' comments above, I don't think he plans to take PRs at this early stage in the project but I thought I would still note a few things I noticed as I looked at the changes in this PR.

@@ -1,141 +1,147 @@
#include <EasyTransfer.h>

Choose a reason for hiding this comment

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

Pretty much every line in the file is marked in the diff as being touched by this commit because the author's editor changed line endings from Unix to DOS. I wouldn't expect someone submitting a PR to make this type of change unless such a change was previously agreed upon by the project maintainer.

Choose a reason for hiding this comment

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

For some reason submitting PRs with github's editor from the website does this, giving the message "We’ve detected the file has mixed line endings. When you commit changes we will normalize them to Windows-style (CRLF)."

}


float radToDeg(radians) {

Choose a reason for hiding this comment

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

The code no longer builds after these changes are applied. This line is missing the type specification for the radians parameter. A similar mistake was made below for the degrees parameter in degToRad().



float radToDeg(radians) {
return radians * 4068/71

Choose a reason for hiding this comment

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

This line doesn't compile either since it is missing the semicolon at the end of the line. The body of degToRad() is missing a semicolon too.

@adamgreen
Copy link

@harveyaitken I see that you have deleted the repository originally associated with this PR. Should this PR be closed as well? Are you planning to create a new PR for this issue in the future?

@XRobots
Copy link
Owner

XRobots commented Jun 28, 2018

I haven't deleted any repositories - it's still right there.

Yes as I keep saying I'll be writing a completely new version of the code.

@adamgreen
Copy link

Sorry, I meant that comment for @harveyaitken since he has deleted his fork of your repository. It was this fork which contained the original changes associated with this PR.

@mrbesen
Copy link

mrbesen commented Feb 5, 2020

For this reason, any changes in part 2 of the video will be in a separate folder called 'part 2'.

I suggest using tags then. Under your videos or in the README.md you can link the tag-page and every user can see all the versions and browse every code of that version.

As example: here is the link for the tag-page of the linux kernel.

If the link of the Commit id, below the version is to subtile for you, you may want to add a markdown link in the description like this:

[browse code](https://github.com/torvalds/linux/commit/d5226fa6dbae0569ee43ecfc08bdcd6770fc4755)

As far as i can tell there are many people wanting to help with their issues and Pullrequests, but the way the code is organized makes it hard and unrewarding.
Maybe just put up a open google drive or dropbox folder with the parts in folders as you like to make it easy for unexperienced people and on github make it "the propper" way.

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.

5 participants