Skip to content

Add a parameter Snnn to M92 that indicates the microstepping factor#247

Merged
dc42 merged 2 commits into
Duet3D:devfrom
wilriker:M92-microstep-parameter
Jan 14, 2019
Merged

Add a parameter Snnn to M92 that indicates the microstepping factor#247
dc42 merged 2 commits into
Duet3D:devfrom
wilriker:M92-microstep-parameter

Conversation

@wilriker
Copy link
Copy Markdown
Contributor

@wilriker wilriker commented Jan 9, 2019

This closes my request at: https://forum.duet3d.com/topic/8225/m92-parameter-to-indicate-microstepping

I was too impatient again. ;-)

I chose the parameter to be Fnnn just because I wanted to avoid Mnnn because of M-codes and X or U are already taken by axis names. So this is microstepping-Factor.

@dc42
Copy link
Copy Markdown
Collaborator

dc42 commented Jan 9, 2019

Thanks for your PR. Suggestions for improvement:

  • I don't like the use of F for the microstepping, F rarely means anything other than feed rate. How about S for Stepping instead?
  • Please use a uint32_t and GetUIValue to get the value of the new parameter, instead of int32 and GetIValue
  • The interface to Platform is already rather fat and I don't like creating a new member function that is almost a duplicate of the old one. I suggest one of the following:
    (a) do the microstep conversion calculation within the M92 code in GCodes2.cpp instead of in Platform. You can use the existing Platform::GetMicrostepping function to fetch the current microstepping of the drive.
    (b) pass the new microstepping value as an extra parameter to Platform::SetDriveStepsPerUnit, or pass 0 if it wasn't specified. Then Platform::SetDriveStepsPerUnit can do the conversion if 0 wasn't passed.

Also unite both SetStepsPerUnit methods into one.
@wilriker
Copy link
Copy Markdown
Contributor Author

I had a hard time thinking of which parameter to actually use. I was also hesitant to use F due to its usual meaning of feed rate but also S seemed not the ideal solution to me (I would have preferred X or U but they are both reserved for axis names). Anyway, I changed it as proposed to S.

Also changed to uint32_t. I would have chosen that in the first place but must have missed that it exists.

And I decided that I unify the two SetDriveStepsPerUnit methods because it had the least code duplication since in M92 handling there are separate code paths for drives and extruders I would have to do the calculation in both loops. This way it is still done for each drive but just in one place in the source.

@wilriker wilriker changed the title Add a parameter Fnnn to M92 that indicates the microstepping factor Add a parameter Snnn to M92 that indicates the microstepping factor Jan 11, 2019
@dc42
Copy link
Copy Markdown
Collaborator

dc42 commented Jan 14, 2019

Thanks for making those changes. Please re-submit your PR with target branch 'dev' because that is the branch I am now using for development.

@wilriker wilriker changed the base branch from v2-dev to dev January 14, 2019 19:40
@wilriker
Copy link
Copy Markdown
Contributor Author

Done.

@dc42 dc42 merged commit 50e3c9d into Duet3D:dev Jan 14, 2019
@wilriker wilriker deleted the M92-microstep-parameter branch January 14, 2019 19:58
@wilriker
Copy link
Copy Markdown
Contributor Author

For sake of completeness: I added this parameter to reprap.org

@dc42
Copy link
Copy Markdown
Collaborator

dc42 commented Jan 31, 2019

Thanks!

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.

2 participants