MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
263 stars 133 forks source link

EEPROM write after B06 and B08 Commands #528

Closed madgrizzle closed 5 years ago

madgrizzle commented 5 years ago

The firmware does not currently write steps to EEPROM after the completion of a B06 or B08 command, both of write directly modify the steps value of the encoder. Normally this isn't a problem since steps get written after a move occurs, but during software testing I discovered I had to continuously reset my chain lengths every time I restarted webcontrol because the were set to 0,0 in EEPROM. This fix sets sys.writeStepsToEEPROM to true after those commands are issued so the new steps value will be written to EEPROM.

MaslowCommunityGardenRobot commented 5 years ago

Congratulations on the pull request @madgrizzle

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

madgrizzle commented 5 years ago

Does this firmware change affect kinematics? No a) If so, does this change require recalibration? No b) If so, is there an option for user to opt-out of the change until ready for recalibration? N/A If not, explain why this is not possible. c) Has the calibration model in gc/hc/wc been updated to agree with firmware change?N/A d) Has this PR been tested on actual machine and/or in fake servo mode (indicate which or both)? No.. can't readily test stock firmware at the moment, but likelihood this causes an issue is extraordinarily low

blurfl commented 5 years ago

@madgrizzle , here's something more to look at in the B06 command.

When the B08 command changes the chain lengths it includes a line to alert the kinematics of the change:

kinematics.forward(leftAxis.read(), rightAxis.read(), &sys.xPosition, &sys.yPosition, 0, 0);

The B06 command does not do that. Because of that, a 'keeping up' alarm will arise if the first movement after a B06 command is greater that the alarm limit. Too, the sled position is not updated on the GC screen. Presently GC only uses B06 in setSprocketsVertical.py, so the issue doesn't arise. Would it make sense for B06 to update the kinematics as well, though?

To see the problem,

madgrizzle commented 5 years ago

I updated patch to add the kinematics.forward. Nice catch, @blurfl!

MaslowCommunityGardenRobot commented 5 years ago

Time is up and we're ready to merge this pull request. Great work!