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

distPerRot Not Used Anymore and Other Questions #461

Open
madgrizzle opened this issue Aug 21, 2018 · 11 comments
Open

distPerRot Not Used Anymore and Other Questions #461

madgrizzle opened this issue Aug 21, 2018 · 11 comments

Comments

@madgrizzle
Copy link
Contributor

First, it appears that the software no longer uses distPerRot in any calculations ever since RleftChainTolerance and RrightChainTolerance were introduced. However, we do need a new value that's similar to distPerRot. We need an accurate chain pitch diameter which is calculated as chain pitch / sin(180/number of teeth). I don't know the impact of changing setting names and what it does to eeprom or whatever.

How should we proceed with making the chain pitch diameter available?

@BarbourSmith
Copy link
Member

Isn't there a new variable called something like distPerRotR or something like that which computes the distance per rotation without the chain correction factor?

@blurfl
Copy link
Collaborator

blurfl commented Aug 21, 2018

Keep in mind that while there are different ways to measure the radius depending on whether you're interested in the location of the pin or the link of the chain, the chain that the sprocket stores is affected by the tolerance factor even when it is stored on the teeth as the teeth gather in or release the chain.

@madgrizzle
Copy link
Contributor Author

Bar, I don't see a disPerRotR.. just distPerRot and it's not used in any routines any longer.

Blurfl, just when I thought it was safe to go back into the water.. I don't know for certain which is correct (to use chain tolerance factors or not) and it's not a big source of error either way. I feel somewhat confident that when we calculate the chain over sprocket, the diameter (and hence radius) we should be using chain chain pitch / sin(180/number of teeth), rather than the current method of using chain pitch * number of teeth * chain compensation. Maybe we should still use the RleftChainTolerance and RrightChainTolerance values to calculate the chain stored, but the radius.. my head hurts.

@BarbourSmith
Copy link
Member

I think I'm thinking of these lines in Settings.cpp:

    sysSettings.distPerRotLeftChainTolerance = 63.5;    // float distPerRotLeftChainTolerance;
    sysSettings.distPerRotRightChainTolerance = 63.5;    // float distPerRotRightChainTolerance;

They were added in #422

I guess maybe my pointing that out was redundant because them being added is the reason distPerRot is not used any more.

Maybe the right place to start is to remove distPerRot, that seems like a safe pull request because if we're not using it it shouldn't be in there

@madgrizzle
Copy link
Contributor Author

madgrizzle commented Aug 21, 2018

Yeah, I just didn't know if that if just deleting it will screw something up with eeprom.. I don't know how the values are saved to eeprom is all.

@blurfl
Copy link
Collaborator

blurfl commented Aug 21, 2018

The thing is that the sprocket moves 10 links of chain per rotation. If the chain is stretched or worn, it is still 10 links that are moved. The radius doesn’t matter, but we shouldn’t use either radius to calculate how much chainis moved.

@BarbourSmith
Copy link
Member

I agree @blurfl we shouldn't be using the radius, we should be using the number of teeth to make that calculation

Looking at settings.cpp it looks like we write the whole structure directly to the eeprom with 140: EEPROM.put(340, sysSettings); so taking the variable out of the structure would give us trouble, but we should be safe to take it out anywhere else it exists and a comment in the structure that it is a legacy variable would be nice

@davidelang
Copy link
Contributor

davidelang commented Aug 22, 2018 via email

@madgrizzle
Copy link
Contributor Author

distPerRot is still used to set kinematics.R for use in quadrilateral calculations.

@c0depr1sm
Copy link
Contributor

Thanks to madgrizzle's commits in his branch. We can see how this can get in order. I think we also need to clean up to increases readability and that might help more contributors jump in. So I forked, started reviewing the code, changing labels and adding the improvements I see in the MaslowCNC master, as well as preparations by contributors. We'll see where that goes...

@BarbourSmith
Copy link
Member

Yes!!!! Clean up and documentation is a GREAT thing to do!

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

5 participants