Fix MeatPack with per-serial-port instances

[X-Ryl669]

Description

I made a mistake in my previous implementation. I had encapsulated Meatpack over the multiserial class.
This causes mix up and issues in meatpack's internal decoding state between serial ports when both are used.

This implementation attach meatpack to each serial port individually (in fact, it allows to enable meatpack per serial port instead of globally).
Since each serial port will have its own instance of Meatpack's internal decoding state, it's not possible anymore that one port breaks the other. Also, it's now possible to disable Meatpack on the TFT port for example (unlikely to speak Meatpack) while still having the USB port or UART port using Meatpack.

Requirements

It's a generic code. MEATPACK must be enabled to have any effect.

Benefits

See above

Configurations

See attached file for the modifications

Related Issues

It's another take at #21301. The idea in #21301 is to cache the meatpack decoding but mess up are still possible when both port sends Meatpack escape code since a single Meatpack's instance is used. I think my approach is more correct, even if has a larger impact on code size.

@rhapsodyv Can you check it and comment, please ?

[X-Ryl669]

@ellensp It's more a bugfix than an improvement. It fix #21304, should fix one of the issue reported in #21010 and maybe #21244

[thisiskeithb]

I added a "Needs Discussion" label based on your comment in #21301 since we have two solutions.

[rhapsodyv]

Yes, if we decide enabled/disable meatpack per serial port, we need this PR.

But yet it has the issue with the parser holding internal state.

You need to add the options to the configuration for the user enabled/disable it too.

And most importantly? Did you test it?

[X-Ryl669]

Is there a documentation of Meatpack protocol somewhere, or is it just this code as the documentation ?
Because, to answer your question, we must know if:

1. Serial port 0 send one byte X
2. Serial port 1 send one byte Y

Does meatpack can interpret XY as something special or does it says "it's Y so it's not for me, let output it transparently, X is for me so let's glob it for my decoder" ?

Reading the code, I think meatpack does not do that, it mixes things up so an approach that's just splitting at the output of the decoding will likely fails if the input is garbage/interleaved.

[thinkyhead]

MeatPack as a stream protocol gets one instance per serial port (that wants it) and each instance can be enabled and disabled independently of the others. It is disabled by default until the start sequence arrives, and it remains enabled until the end sequence arrives. It's unclear how to handle a port stuck in MeatPack mode if the sender just "goes away," but presumably on reconnect the plugin will say something again, or we can choose to turn it off on any detectable fresh reconnect.

[thinkyhead]

The protocol is generally described here https://github.com/scottmudge/OctoPrint-MeatPack and when this first arrived I validated the code in Marlin by reading the corresponding (and not very long) MeatPack compression code to make sure that I grokked it completely.

[thinkyhead]

If I understand correctly, the multi-serial code can actually disregard the MeatPack aspect of the serial ports that it wraps, and therefore if we use simple inheritance to add MeatPack to a base class then the Multi-Serial class won't need any special case for extended classes like MeatPackSerial.

[X-Ryl669]

Alright. It's not 100% transparent, yet, since if you send 2 0xFF bytes, then it'll not forward them. It might break binary stream here.

the multi-serial code can actually disregard the MeatPack aspect of the serial ports that it wraps

True.

therefore if we use simple inheritance to add MeatPack to a base class then the Multi-Serial class won't need any special case for extended classes like MeatPackSerial

Yes, that's the idea. But we still need something to enable the base class and that the idea of this PR. If MEATPACK_ENABLED_ON_SERIAL_PORT_0 is defined, then the serial class is MeatPack<TheInitialSerialClass> else it's TheInitialSerialClass

This is just a preprocessor/typedef work going on here to select the right type to use for the final serial class, but adding MeatPackSerial in the hierarchy here should be transparent.

[thinkyhead]

Check whether binary stream bypasses everything else, including MeatPack and the queue feeder, when it is in effect.

[X-Ryl669]

I don't understand. Do you want me to check, or you've checked it's ok ?

[thinkyhead]

Note that it is not expected that a binary stream should arrive as part of a print job, but should be a fairly atomic operation. And, if a host were sending a binary stream — either alone or in tandem with a G-code to initiate it — it's most likely that MeatPack would be temporarily suspended, as it certainly would not want to be involved in a transfer of data which is not G-code. That said, a MeatPack-compressed stream would be a nice extra optimization for a G-code binary file transfer.

[X-Ryl669]

It looks like it's not bypassed. Not sure it's good either.
My reasoning is this:

1. we have binaryStream[card.transfer_port_index.index].receive(serial_state[card.transfer_port_index.index].line_buffer); in the queue code
2. card.transfer_port_index is set to a queue's ringbuffer port, so it's definitively a serial index
3. serial_state.line_buffer is build up via the serial class so it'll pass through MeatpackSerial if it's built in.

[X-Ryl669]

Meatpack will perform poorly on a binary stream. It's currently good because the alphabet of the data to encode is very small and repetitive. On a (pseudo) random alphabet, it'll escape almost all its code and instead, will consume twice the binary space (if I understand the encoding algorithm correctly)

And if it's not switched off, then it'll break on 0xFF 0xFF like sequences (it's not that rare in a binary stream). Or worse, might enable itself if it finds the 0xFF 0xFB sequence, with catastrophic consequence.

[p3p]

That said, a MeatPack-compressed stream would be a nice extra optimization for a G-code binary file transfer

binary file transfer uses heatshrink for stream compression.

@X-Ryl669

The version of Binary File Transfer in Marlin at the moment just uses the line_buffer as a working buffer rather than allocate its own memory, it is completely independent in general.

I'm still working on the finished version, but with all these serial changes I'm not sure how to integrate at the correct point anymore.

[X-Ryl669]

I would like to welcome our new Grep's overlord! 😉

[rhapsodyv]

I think we missed a sanity check for enabling MEATPACK_ON_SERIAL_PORT_2 without a serial_2

[X-Ryl669]

When Scott frenetic commits slows down, I'll add the check. My initial attempt was not logical since it relied on user not removing the #ifdef stuff.

[X-Ryl669]

diff --git a/Marlin/src/inc/SanityCheck.h b/Marlin/src/inc/SanityCheck.h
index 400ed51d93..186be664e5 100644
--- a/Marlin/src/inc/SanityCheck.h
+++ b/Marlin/src/inc/SanityCheck.h
@@ -3346,6 +3346,9 @@ static_assert(   _ARR_TEST(3,0) && _ARR_TEST(3,1) && _ARR_TEST(3,2)
   #elif NONE(MEATPACK_ON_SERIAL_PORT_1, MEATPACK_ON_SERIAL_PORT_2)
     #error "MEATPACK is now enabled with MEATPACK_ON_SERIAL_PORT_1, MEATPACK_ON_SERIAL_PORT_2, etc."
   #endif
+  #if ENABLED(MEATPACK_ON_SERIAL_PORT_2) && !DISABLED(SERIAL_PORT_2)
+    #error "MEATPACK_ON_SERIAL_PORT_2 is only valid when SERIAL_PORT_2 is defined"
+  #endif
 #endif

 /**

Like this ?

[thinkyhead]

When Scott frenetic commits slows down, I'll add the check.

When there's lots of activity throwing new commits in the way, I find that instead of fetch-and-merge (adding yet another ugly merge commit) this tends to avoid conflict, or at least minimizes it.

Undo commit… Stash … Fetch … Pull … Restore Stash … Commit … Push.

If the push fails, wait a few seconds and…

Undo commit… Stash … Fetch … Pull … Restore Stash … Commit … Push.

[X-Ryl669]

I was joking. You're committing very fast, I can't keep up. So I'm monitoring the commit time and when it's old enough, I'm retrying...

I'm rebasing, it works better than fetch & merge in that case, unless you've updated the same part of code as I did. It's a single command (git commit -a && git pull --rebase && git push) and it works most of the time.

[thinkyhead]

Note that MSerial# items have not been renumbered because they are sometimes specified by appending the SERIAL_PORT value to "MSerial". Clearly, in none of those cases could the serial port setting be -1 (e.g., native USB) because "BLAH-1" is not a good macro name, and the INCREMENT macro can't take negative numbers either. Given that none of these use native USB, they could actually moved up by using _MSERIAL(INCREMENT(X)). It's a semi-arbitrary choice, but they roughly correspond to the zero-indexed Arduino objects Serial, Serial1, Serial2, etc.

[Sebazzz]

As mentioned on #21010, this works well on: CR-6 SE, with BTT SKR CR6 board (USB software serial + BTT TFT via hardware serial).

[thinkyhead]

Thanks for sticking through and getting this solved. Hopefully this brings us closer to stability and a higher confidence in the next release. The codebase deserves a lot of refactoring, but for now it's all about patching existing issues. I'll go through issues over the coming days and compile a list of the problems that need the most attention.

[Sebazzz]

@thinkyhead If there is anything I can do, I'm happy to. Core serial buffering protocol handling might be a bit out of my league, but I'm happy to look into other things.

[GhostlyCrowd]

This is working great, as expected. Just as a follow up as i am home and able to test now.