Cleaning up the firmware: labels, documentation, and catching up on previous attempts

Software Maslow Firmware
1

I said I would do it, and I think I did it:
The latest commit to my fork is the sum of a clean-up work.
Several changes split in several commits. And notably addition of 4 schematic drawings in SVG format(see firmware/documentation)

Ok, I admit I did a little more: I added helix (G2 and G3 with z move) support to the arc() function and renamed it arcXYZMove()… But it should be backward compatible.

I invite every one to check it thoroughly. I am personnaly using it and am going to use it for some time before I do anymore work. It is necessarily a beta version just because it has not so many users nor so many running hours.

I know we usually do fork/branch/commit/pull. And we can all see that madgrizzle is working on an optical calibration feature. So I worked as carefully as I could.

Here is the summary.
a) Includes 4 schematic drawings in SVG format(see firmware/documentation)
b) was mainly focussed on the specific Maslow firmware controls (GRBL uses step motors. Maslow cnc uses DC motors + rotary encoders and servo loops.)
c) has renamed many labels to become self explanatory
d) clearly splits the (x,y,z) GCode coordinate space and the (left,right,z) motor control space
e) contains about every clean-up efforts I could gather from others that were not yet merged in the master branch (I gave credits in the commits and comments)
f) Added helix (G2 and G3 with z move) support to the arc() function and renamed it arcXYZMove()
g) replaced the G1 call in arcXYZMove() with a straightLineMove() to simplify it (straightLineMove() is the new name of coordinatedMove())
h) grouped and re-ordered the cnc_ctrl_v1.ino setup() with equivalent functionnality.
i) include the top beam angle settings and calculation proposed by madgrizzle, So the syssettings eeprom table is now rev 5 in my branch. (default value is 0 to be compatible with current ground control)
j) cleaned-up the kinnematics class for better readability, hiding what really is private

known isues:
a) I renamed two funtions into the encoder.cpp: read() and write(). And this would be a bad practice to change anything into a library so mature as that one.

1 Like
2

wow awesome!, especially the g2/g3 commands! maybe post where we can download this new version to beta test?

1 Like
3

Tx. Edited the post with a link :slight_smile:

1 Like
4

Great work!!! :+1::+1:

In general I try to encourage as many small pull requests as possible (ie one for just adding the schematics)…etc to make it easier to test but with our new monthly release schedule there is a lot more time for testing between releases so maybe we can handle bigger ones now

5

In general, a pull request per significant change is a good idea (do them in separate branches)

but in a case like this where you are doing label/function name changes, it’s good to have them all in one PR.

You did do your changes in multiple commits, which is good. Usually, you have commits that do one thing, then another commit to fix it, then work on something else, then go back and fix another bug in the first feature…

If you can take the time to use git rebase to re-order the commits and squash fixes into the original commit (essentially ‘modifying history’ so that the PR looks like you were a perfect developer, never making any mistakes, everything being a logical step based on the prior one, etc), it makes the results very easy to read/evaluate. It is a bunch of work, but if you can do it, it really helps.

2 Likes