Desuuuu / Marlin

Optimized firmware for RepRap 3D printers based on the Arduino platform. Modified with a new DWIN T5UID1 touchscreen implementation.
GNU General Public License v3.0
31 stars 10 forks source link

Add UBL support (https://github.com/Desuuuu/Marlin/issues/8) #12

Closed mrv96 closed 3 years ago

mrv96 commented 3 years ago

When you will approve this PR i have a new one ready to submit that fixes https://github.com/Desuuuu/Marlin/issues/6

Desuuuu commented 3 years ago

I have a few issues with this PR:

I'll make the changes once I get your input on these points (mostly the first one).

mrv96 commented 3 years ago

Your doubts are very reasonable. Let's start with point two: I needed to use inject_P because the idle() in the while during gcode queuing, it is not able to handle tough gcodes like G29. If you try to avoid inject_P, the printer will stop forever because it is not able to digest the G29 command. A raw solution could be to replace idle() with loop(), but if possible i want to avoid to touch standard Marlin code.

The first point follows the second one: in principle injected gcodes should be executed before the standard queue, but since M420S1 and M420L15 are not dependent by the execution order, i put them at the beginning to be sure that they are executed before the last M500 command.

Concerning the last point: M500 is needed because M420 commands, hence a dedicated G20S0 command is not necessary at all. Moreover G20S0 would be another gcode which contibute to full the standard queue, and cannot be injected because its execution depends by the previous commands. It must be executed in the right order.

I hope that I was clear.

P.S: consider to merge directly https://github.com/Desuuuu/Marlin/pull/13 discarding this one, because it contains the commits in this PR.

Desuuuu commented 3 years ago

Well, the queue is limited to BUFSIZE commands, so if you're trying to queue more than that and block until all of your commands are queued, you're indeed sending Marlin into an infinite loop. It's not particularly related to G29.

The thing is, running M420S1 and M420L15 (which loads a saved mesh, I still don't understand why that's even there) serves no purpose. ABL is already being enabled upon returning to the LEVELING_AUTOMATIC screen.

Taking that into consideration, there's only 4 commands that need to be queued (G29P1, G29P3, G29P5C and M500) and that should fit into the default buffer (since the queue is empty at this point).

mrv96 commented 3 years ago

Ok. idle() effectively doesn't handle gcodes, so I don't understand why Marlin is implemented in this way. Anyway this is not a problem related to this PR.

I saw only now that M420S1 needs a valid mesh. Sorry. My fault. It must be shifted.

UBL leveling needs also the two M420 commands (or at least M420S1), hence there are more than 4 commands queued. M420S1 enable ABL, doesn't load anything

Desuuuu commented 3 years ago

It should work fine with the changes I just made. Please let me know what you think about it.

mrv96 commented 3 years ago

M420S1 is needed to enable ABL. It doesn't load anything: https://marlinfw.org/docs/gcode/M420.html. It is needed with both UBL and bilinear. IMHO it is required, or am i wrong?

M420L15 is a good default feature IMHO. What do you think about?

Desuuuu commented 3 years ago

I know that M420S1 doesn't load a mesh, but M420L15 does (it's documented on the exact link you sent). The parameter for fade height is Z.

Queuing M420S1 is redundant because after the procedure ends, ABL will be enabled by that code: https://github.com/Desuuuu/Marlin/blob/eb7ce1a8c5ec51cf71ab659eeae3dc52f933b3de/Marlin/src/lcd/extui/lib/dgus_reloaded/DGUSSetupHandler.cpp#L154

mrv96 commented 3 years ago

Ok. I missed setLevelingActive method. M420L15 is a mistake. I want to put M420Z15. Sorry I'm doing a lot of coding and a lot of mistakes

mrv96 commented 3 years ago

I'm used to use G29 UBL gcodes, not M420. I'm not familiar with it. Sorry again

Desuuuu commented 3 years ago

All good.

Regarding fade height, we could indeed set one but for now I think it's fine to let the user do it (in their start gcode for example).

It could be done somewhere else too, like when changing leveling state.

mrv96 commented 3 years ago

Ok, not a problem. I'm coding thinking to a standard, low level 3d printers user. I would have enabled that command because maybe, the printed parts can be better, but for the moment we can avoid it, it is not absolutely necessary.

mrv96 commented 3 years ago

Start G-code could be a good option

mrv96 commented 3 years ago

If done in another part of the code it requires an additional M500