[WIP] Asynchronous serial

[linvinus]

• Your changes are not possible to do through a plugin and relevant
  to a large audience (ideally all users of OctoPrint)
• If your changes are large or otherwise disruptive: You have
  made sure your changes don't interfere with current development by
  talking it through with the maintainers, e.g. through a
  Brainstorming ticket
• Your PR targets OctoPrint's devel branch if it's a completely
  new feature, or maintenance if it's a bug fix or improvement of
  existing functionality for the current stable version (no PRs
  against master or anything else please)
• Your PR was opened from a custom branch on your repository
  (no PRs from your version of master, maintenance or devel please),
  e.g. dev/my_new_feature or fix/my_bugfix
• Your PR only contains relevant changes: no unrelated files,
  no dead code, ideally only one commit - rebase and squash your PR
  if necessary!
• Your changes follow the existing coding style
• If your changes include style sheets: You have modified the
  .less source files, not the .css files (those are generated with
  lessc)
• You have tested your changes (please state how!) - ideally you
  have added unit tests
• You have run the existing unit tests against your changes and
  nothing broke
• You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

Resolve #2834 #2834

How was it tested? How can it be tested by the reviewer?

compile marlin with following options

#define BAUDRATE 500000
#define BLOCK_BUFFER_SIZE 64
#define BUFSIZE 32
#define TX_BUFFER_SIZE 32
#define RX_BUFFER_SIZE 2048
#define ADVANCED_OK

print some cylinder at maximum printing speed, compare print quality

Any background context you want to provide?

no

What are the relevant tickets if any?

#2834

[linvinus]

looks like travis still using my previous branch. error on line 3170 but in my repository this line is different https://github.com/linvinus/OctoPrint/blob/5b6bd5a7f53bf815a8eef0cc1b73ee0dbd7d189b/src/octoprint/util/comm.py#L3170

[linvinus]

@foosel now working good with maintenance branch

[linvinus]

@foosel hi, i have made a little improvement - clear send queue on canceling.
Should I hand this over to my branch?

[foosel]

I found some things that need clarification and some things that need changing. Could you take a look?

[foosel]

New variables should be in snake_case - all lower case, words spaced with an underscore.

I know that some of the existing variables in the code are still camelCase, but they are slowly being migrated as well.

Please never start a variable name with an upper case letter, that makes it look like a class.

It would also help to know what these variables mean, after having read through all the other changes I'm still unclear on it.

Finally, whether this feature is actually supported or not should receive an additional feature flag (similar to busy support). Feature flag should be enabled by default, but _AdvancedOkSupported should only ever become True if support is detected and said flag is True too. That will allow to disable it and fall back on pure ping pong in case of broken firmwares and/or very specific bugs in this implementation.

[foosel]

What about STATE_PAUSING, STATE_RESUMING, STATE_CANCELLING? Printing state is more than just STATE_PRINTING.

[linvinus]

this code filling the send queue with respect to marlin buffers, in non printing state older synchronous mode should be working fine

[foosel]

The aforementioned states are also printing states however.

[foosel]

This feels very dangerous and is a huge deadlock waiting to happen. It shouldn't happen as long as sensible data is being exchanged, but that's not something we can count on with finicky cabling and controllers. This needs some kind of fallback to ensure we don't get stuck in this loop forever.

[linvinus]

you are right, self._advanced_ok_last_line must be retested to -1 at communication timeout, if we loose synchronization

[foosel]

What is cruLineTimeout supposed to mean? Typo of cur? Write out words fully please, for readability, and in snake_case => current_line_timeout.

[foosel]

I'm completely unclear on what this whole block does. Needs more comments ;)

And busy polls are always a red flag - do we really need that here? Comments might clear up why, if so.

[linvinus]

sorry haven't enough time now, simply in asynchronous transaction mode, synchronization through _clear_to_send queue is not working at all.

here we simply waiting until marlin free buffer, or timeout will occurred.

[foosel]

Keep in mind that I will have to maintain this code going forward - this needs proper documentation so that that will be possible.

[foosel]

From the looks of it this should rather be

def parse_advanced_ok_line(line):
    """
    Parses the provided ADVANCED_OK line.

    Format:
        ok N<last processed line> P<free planner slots> B<free command slots>

    Args:
        line (str): the line to parse

    Returns:
        (dict or None) A dict containing the last processed line number (``N``), the amount of free 
        slots in the motion planner buffer (``P``) and the amount of free slots in the command buffer (``B``), 
        or None if no match could be made.
    """ 
    # ...

And something about the whitespace seems to be fishy in the function as well.

[foosel]

@foosel hi, i have made a little improvement - clear send queue on canceling.
Should I hand this over to my branch?

That sounds like it should be a separate PR. Let's concentrate on what we have here first.

[Evg33]

Hello !
What about resolve conflicts ?

[linvinus]

Hi guys. Until the 5th of August I'm on vacation.
@Evg33 which conflicts do you mean?

[Evg33]

@linvinus

Conflicting files
AUTHORS.md
src/octoprint/util/comm.py

[linvinus]

@Evg33
With which version?

travis don't see any conflicts

[Evg33]

[linvinus]

@Evg33 Ohh, in mobile version i don't see that.

[linvinus]

@Evg33 resolved

[linvinus]

Currently I have noticed the following:

1. ATMEGA 16MHz can not printing faster than 35mm / s when the LA,JD and S-curve is turned on and model have a lot of circles

For speed test i use the following model https://www.thingiverse.com/thing:1185928
this model don't have straight lines, and almost don't have transfers, which is why the whole gcode consists of many small lines

Model was sliced in Cura with following settings
Layer width 0.4
Layer height 0.2
Shell count 5
Scale 50%
If printing speed is over 35mm/s then in terminal in ADVANCED_OK reports you will notice that planner buffer will be empty very often (P: will high) This means that the nozzle is moving faster than Marlin can calculate a new trajectory. And i'm sure that this is not because of slow serial transfer, because at this moments RX_BUFFER have around 40 additional commands ready for parsing.
Completely disabling the LCD, and other hacks did not give a noticeable increase in performance.

Marlin have SLOWDOWN option to resolve that cases but it will slow down printing and anyway printing quality will be poor.
Something like this

2. From octoprint side, the information about buffers in ADVANCED_OK is useless, only the line number is needed. If planner buffer goes down octoprint will not be able to react on that fast enough.
   The best what we can do is better fill marlin RX_BUFFER, so this buffer will be as a piece of real gcode file, so marlin will have all it needs for successful printing.

3. In marlin the BUFSIZE is used to protect against transmission errors, but at high speeds with asynchronous transmission, it is useless. if a transmission error was occurs, while the octoprint will resends a broken command the printer will stop the nozzle.
   Currently re-sending with asynchronous transactions is broken, because it also require to change logic on marlin side.

4. From octoprint side more importantly is to have information about filling of RX_BUFFER_SIZE , but there is no information about it in ADVANCED_OK

[Evg33]

@linvinus Thank you !!!

[linvinus]

@Evg33, no problem. let me know if you have any questions. It seems that you are the first man who will testing this patches, i will appreciate for any feedback!

[foosel]

@linvinus I have to admit that I lost track of this somewhat after my initial review... I see that you pushed some changes in the meantime, but some points appear to remain unaddressed.

Additionally from your comment here it sounds like this isn't ready for prime time yet due to firmware issues, or did I misunderstand that?

[linvinus]

@foosel hi!
patch it self is working, but i found that on 8bit processors marlin performance is not enough to print circles faster than 35mm/s.
This is doesn't mean that patch is useless, it steel helps a lot on more simple models, like 3D benchy.

Currently i'm planning to replace 8-bit processor with STM32F1 and then compare print quality.

[DiegoGiovany]

+1 for this... printing over wifi at 300mm/s on a tinyfab cpu (LPC1769) in a UP BOX with the linvinus version, with the oficial version, I have multiple slowdowns and blobs, and even comunications errors...

[foosel]

Please don't leave "+1" comments but instead vote using the built-in Github reactions:

[foosel]

There's still a whole bunch of open points.

[foosel]

The list of buffered commands still seems way too long. M104 is definitely not buffered. It might be that some of the other ones are buffered on your specific fork, but not in general. We should really stick to universally buffered commands here which - again - are G0 through G3 and G28 through G32. Anything more than that and we risk issues with firmware variants.

[foosel]

starting, pausing, cancelling, resuming, finishing? Probably better: self._state in self.PRINTING_STATES

[foosel]

Please remove unused code

[foosel]

Please remove unused code

[foosel]

You'll have to take into account that a line might have been sent without a number (possible for in-band commands that aren't GCODE, or just for certain configuration options).

[foosel]

needs documentation

[foosel]

rename: self._advanced_ok_last_sent_line

[foosel]

rename: self._advanced_ok_last_ackd_line

[foosel]

rename: self._advanced_ok_buffered_commands

[linvinus]

FIG1

@foosel hi!
i saw your comments, but before continue discussing code let me describe where the problem is, what this patch trying to resolve.

Please have a look at figure 1.
On left side we have octoprint on the right firmware (marlin).
Because ram in firmware is too small we can't send whole file, so we send it by chunks - line by line.
On firmware side some commands executing synchronously, that is mean that marlin send OK report only after movements or other physical actions was performed.
(T1 -> T2 -> T3 -> T4 -> T6 -> T7)

Others, instead, goes to special buffer with size of BLOCK_BUFFER_SIZE, and OK report sends immediately after checking crc summ, but before actual physical actions was performed.
(T1 -> T2 -> T3 -> T4 -> T5)

The problem is "poor printing quality like bulbs or unstable line width".

Where there come from?

Because of STL file is triangle based, it unable to describe circles.
The circles actually represented as small lines. Usually slicers allow configure resolution of such lines.
So any non straight lines is printing as such small lines.
This is meas that some times to print one short figure printer must parse a lot of simple short lines.

So to resolve this printing problems we should

1. increase buffers on printer side as much as possible (or place whole file on sd-card)
2. increase speed of data transfer from octoprint to firmware
3. firmware performance must be enough to parse GCODE fast enough

You may notice that all of them is actually hardware related, so simple use powerful processor with big enough ram and you will not have this problems.

If we can't change hardware, what can be done from software perspective?

From software we may optimize protocol between octoprint and firmware.

Even the shortest loop (T1 -> T2 -> T3 -> T4 -> T5) actually too long, this is meas that
(T1 -> T2 -> T3 -> T4 -> T5) >> T6 - nozzle moving faster than new commands arrive
But because filament unable to stop leaking when printer is waiting for next command to arrive, at this position we get bulb and because the pressure in nozzle was reduced next printed line will be with smaller width.

So, instead of waiting for report from T5 step, we may send new commands in advance.

THIS IS WHAT I MEAN BY "ASYNCHRONOUS COMMANDS"

How much? As much as enough space in RX_BUFFER.
Typical GCODE command length is less than 96 bytes (96 is max command length in Marlin), so with buffer of 2000 bytes we may send around 20 commands in advance.

Because RX_BUFFER is flat and commands usually smaller than 96bytes, if we will be smart enough, on octoprint side we may calculate free space in RX_BUFFER and send commands with respect of that.
So, with commands of size 88 bytes we may send in advance around 22 commands.

So, doesn't matter is the command is "buffered" or not in terms of firmware, we send it in advance.
Yes that is mean that user commands like stop or pause will be interpreted by 22 commands later, this is disadvantage of that optimization.

But not all of commands may be send in advance, that is why we have bufferedCommands list, it contain commands that may be send asynchronously without causing error or unpredictable behavior in firmware.
It working for marlin, probably, with some others firmware some of this commands may cause problems. Needs testing and user feed backs.
Any way, the user may adjust this list at its discretion at any time.
Also this new octoprint behavior may be disabled by default.

[foosel]

I'm well aware of the buffering situation in Marlin and similar firmwares (and in fact it's three buffers: rx -> command -> planner) and even had to explain it to current Marlin maintainers ;)

I'm also aware of what this patch is trying to achieve and how it's trying to achieve it. I've been playing with stuff like this long before the ADVANCED_OK format was introduced and stumbled over some crippling bugs in the firmware in the process which I patched (rx -> command not keeping command order).

Remember that I have to wrap my head around code I didn't write myself here, and if you call something "buffered" commands, then I assume you mean those that are officially considered buffered, as in, they go into the planner buffer. Not arbitrary commands. After taking another look at the code I now understood that you are actually concentrating on keeping the command buffer filled, and only that, not the planner buffer.

But not all of commands may be send in advance, that is why we have bufferedCommands list, it contain commands that may be send asynchronously without causing error or unpredictable behavior in firmware.

This is where you've lost me. Things should go straight from RX to Command buffer, in the same order as received, without any changes other than stripping line number and checksum, if set, and potentially requesting a resend first. Nothing should be executed here and hence no command should be blacklisted from getting sent to fill the command buffer. If we are tracking the command buffer here and not the planner buffer, then we shouldn't need a distinction between command types because all commands are the same until it comes to the question, "planner buffer yes or no". And the command buffer will always be executed in order.

                            +-------------------------------------+
        line                |                                     |
+------------------------>  |          RX Buffer                  |
                            |                                     |
                            +------------+------+-----------------+
             "Resend:n"                  |      |
<----------------------------------------+      | checksum & linenr ok, strip
                "ok"                            v
                            +-------------------+-----------------+
                            |                                     |
                            |          Command Buffer             |
                            |                                     |
                            +--------+--------------------+-------+
         "ok"                        |                    |
<------------------------------------+--------------------+ buffered
                                                          |
                            +-----------------------------v--------+
                            |                                      |
                            |             Planner Buffer           |
                            |                                      |
                            +--------------------------------------+

RX to command buffer in Marlin is being done here. No ok generated there other than the one belonging to a resend request triggered by one of the various gcode_line_error lines. After that stuff is in the command buffer (which - in Marlin - gets processed here). RepetierFirmware and Smoothie look similar.

So either I'm missing something crucial here, or this whole buffered list that seems to be main concern here isn't even needed (and misnamed since the term "buffered command" is already reserved for commands that go into the planner buffer). And if I'm missing something crucial here, please tell me what.

[linvinus]

After taking another look at the code I now understood that you are actually concentrating on keeping the command buffer filled, and only that, not the planner buffer.

Absolutely right!

Initially i also playing with planner buffer, trying to make it filled full all the time as much as possible, but as i say on 8-bit processor bottleneck is processor performance.

NOTE 1
There is also one important point: arduino has only one run loop, so GCODE analysis and command execution are always executed one after another.
If we spent a lot of time in parsing stage then no command will be executed - the nozzle will stand.
On marlin side i also have try three different strategies:

1. parse all available commands in RX buffer then execute (default marlin behavior)
2. parse (rx + command buffer) to fill at least up to 1/3 of planar buffer then execute
3. parse one command then execute immediately

There is no big differences between this strategies when processor performance is not enough.
May be there will be differences for mixed commands (short - long - short) but any way printing quality will be unpredictable, some models will be good while others, with the same slicer options, will be bad, as the result users will go crazy.

NOTE 2:
As we decide to send commands in advance, if marlin will get some of them with crc error (at this stage https://github.com/MarlinFirmware/Marlin/blob/0110faa717e642574ade9eb7aed250ee2840e942/Marlin/Marlin_main.cpp#L1021-L1148)
then we will be in trouble, for example:
octoprint already send line 1022
while marlin parse only line 1000 and found crc mistake, then it will flush rx_buffer and send report "resend 999" and read next line in rx buffer, it will expect line 1000 but will get 1020 or some line near (because of linux OS buffers or some other buffers on transition path (USB or WIFI)), then it will report resend 999 again and again.
What that mean? It mean that resend is completely broken - nozzle will stand and quality will be broken.
And we can't do anything with that.
So i think if resend is occurring for user then this is hardware problem and user must resolve it (replace wires, reduce line speed or something like that), we can't help him here.

Because "Command Buffer" - is used only for resend purpose, it useless for us, we may reduce it up to one (BUFSIZE = 1) and instead increase RX_BUFFER as much as possible.

NOTE 3
Now, with NOTE1 and NOTE2 in mind, let's have a look at "advanced_ok" report
<line number> ok P:30 B:0

Now "B" is constant, but "P" is very unstable and non of them is related to free bytes in RX_BUFFER.

So information about buffers in advanced_ok is completely useless. Only line number is good to know.

But then how to send commands in advance with respect to free space in RX_BUFFER? (to prevent RX orverflow)
As workaround for that we can use static value at this line https://github.com/linvinus/OctoPrint/blob/3a5bdb78d093f2d3cf322420a3bcee5181522d61/src/octoprint/util/comm.py#L2261

self._advanced_ok_max_line = self._advanced_ok_last_line + (RX_BUFFER/MAX_CMD_SIZE - 1)

here RX_BUFFER and MAX_CMD_SIZE is firmware constants which,in general, is unknown to octoprint :(

I think we can make additional option for user where he can setup value (RX_BUFFER/MAX_CMD_SIZE - 1) suitable for his firmware.

Now you know everything that is important to know about this patch.

If we are tracking the command buffer here and not the planner buffer, then we shouldn't need a distinction between command types because all commands are the same

you are almost right, the reason why i use bufferedCommands list is that we can't send some commands in advance because otherwise we will loose synchronization

this function is used for fast commands https://github.com/linvinus/OctoPrint/blob/3a5bdb78d093f2d3cf322420a3bcee5181522d61/src/octoprint/util/comm.py#L3187

while this one for slow https://github.com/linvinus/OctoPrint/blob/3a5bdb78d093f2d3cf322420a3bcee5181522d61/src/octoprint/util/comm.py#L3202

[GitIssueBot]

This pull request has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/the-ongoing-usb-conspiracy-theory/6341/85

[Coffee0297]

if you wanna test with something that will be octoprint bottleneck, take a look at klipper. we have to use virtual sd to get speed on circles (70mm/s+).
https://github.com/KevinOConnor/klipper

[fiveangle]

Connecting to original #450 issue...

[foosel]

Closing this as it doesn't look like this will work reliably with the current comm layer and/or firmware implementations, and the review comment were never addressed either. Additionally there are also now projects like https://github.com/chendo/BufferBuddy