search

kliment / Printrun

Pronterface, Pronsole, and Printcore - Pure Python 3d printing host software
GNU General Public License v3.0
2.35k stars 995 forks source link

M110 notation standardization #1378

Closed dMAC95 closed 11 months ago

dMAC95 commented 11 months ago

This standardizes the M110 notation to fall inline with the Prusa MK3 3.12 firmware syntax. This implementation ensures other commands within the string aren't affected by swapping the command and value positions.

"N-1 M110" becomes "M110 N-1" "Some command N47 M110" becomes "Some command M110 N47"

dMAC95 commented 11 months ago

Fixes #1341

rockstorm101 commented 11 months ago

Hi @dMAC95, thanks a lot for contributing! Great job identifying a potential root cause and solution for this issue.

However, I am not 100% convinced on the need for this swapping mechanism. I don't own a printer with Prusa's firmware so I'm not able to either replicate the issue or test solutions myself. Nonetheless, if the issue is as you described and if I understand it correctly, simply hard-coding "M110 N-1" instead of just "M110" should suffice. I created a branch with what I mean here. Could you please test that out and confirm it works? Many thanks in advance.

EDIT: Just in case it is not obvious, in order to test the changes one would need to checkout the fix-m110-prusa branch from my personal repository like:

mkdir test-m110
cd test-m110
git clone -b fix-m110-prusa https://github.com/rockstorm101/Printrun.git .
# proceed with testing
dMAC95 commented 11 months ago

Hi @dMAC95, thanks a lot for contributing! Great job identifying a potential root cause and solution for this issue.

However, I am not 100% convinced on the need for this swapping mechanism. I don't own a printer with Prusa's firmware so I'm not able to either replicate the issue or test solutions myself. Nonetheless, if the issue is as you described and if I understand it correctly, simply hard-coding "M110 N-1" instead of just "M110" should suffice. I created a branch with what I mean here. Could you please test that out and confirm it works? Many thanks in advance.

EDIT: Just in case it is not obvious, in order to test the changes one would need to checkout the fix-m110-prusa branch from my personal repository like:

mkdir test-m110
cd test-m110
git clone -b fix-m110-prusa https://github.com/rockstorm101/Printrun.git .
# proceed with testing

Sure, I'll test your fix. I wasn't sure if M110 appears in any strings containing multiple commands so the intention was to play it safe by creating the swapping method. Being that this error occurs during the start of a new print the solution you provided should be fine.

dMAC95 commented 11 months ago 
https://github.com/rockstorm101/Printrun.git

I tested your solution and it works, I downloaded printrun and ran the MK3 3.12 and while it printed the first time, it failed to print a second time / subsequent times. I updated the printcore code to copy your modifications on the branch you provided and it was able to print successfully on subsequent prints with no line mismatch errors being output.

dMAC95 commented 11 months ago

Testing with MK3 3.13.0 firmware...

dMAC95 commented 11 months ago

Confirmed the bug is present with MK3 3.13.0 firmware and is resolved by this solution.

dMAC95 commented 11 months ago

Only concern is the command being sent now appears to be "N-1 M110 N-1" I hope the duplication of N-1 doesn't cause issue with other printer firmware

rockstorm101 commented 11 months ago

Thanks a lot for taking the time to test this and report feedback. Much appreciated.

Only concern is the command being sent now appears to be "N-1 M110 N-1"

Good point here. It is definitely not elegant. It is no problem for the Marlin that I have but it definitely doesn't make much sense to have it that way. Would you mind testing that branch again (I just pushed another commit to eliminate that line numbering and check-summing) and confirm everything works as expected still?

dMAC95 commented 11 months ago

Thanks a lot for taking the time to test this and report feedback. Much appreciated.

Only concern is the command being sent now appears to be "N-1 M110 N-1"

Good point here. It is definitely not elegant. It is no problem for the Marlin that I have but it definitely doesn't make much sense to have it that way. Would you mind testing that branch again (I just pushed another commit to eliminate that line numbering and check-summing) and confirm everything works as expected still?

That is a basic change, it should just work, and I don't see a need for testing. We understand the code being sent and the missing checksum shouldn't impact much. (From my understanding). If you really want me to I can test it though, I'm 99% certain the implementation is good 👍

rockstorm101 commented 11 months ago

If you really want me to I can test it though, I'm 99% certain the implementation is good 👍

Fair enough. if you add that second adjustment I'll merge this PR. Also please reinstate the newline at the end of the file.

dMAC95 commented 11 months ago

If you really want me to I can test it though, I'm 99% certain the implementation is good 👍

Fair enough. if you add that second adjustment I'll merge this PR. Also please reinstate the newline at the end of the file.

Will do, thankyou