Fiz Meat Pack with Multi Serial and NO_TIMEOUT

[rhapsodyv]

Meat pack has an internal buffer to handle serial data... but it uses only one buffer for all serial. When using multi serial, it will mix data from all serials, mixing commands and responses. This PR makes it create a buffer for each serial.

NO_TIMEOUT is broken too, because of a missing return false; on any_serial_data_available.

This PR fix for me meat pack + multi serial + NO_TIMEOUT.

It fix a follow up for new serial_index_t with M28 (#21304)

Maybe @X-Ryl669 has another fix for this issue.

@Sebazzz is having one more issue with this combination of options plus Usb Serial on F1 board. I currently don't have a F1 board with UsbSerial, so I fixed the errors I could replicate using multi serial on my boards.

May fix #21010 and #21244

Fix #21304

[rhapsodyv]

Video of the issue that this PR solves

[GhostlyCrowd]

This fixes the multi serial issue for me with MeatPack, and i can re-enable my TFT serial port.

[rhapsodyv]

But everything works?

if not, can you share your configs?

[GhostlyCrowd]

But everything works?

if not, can you share your configs?

Everything is working as it was before, my TFT is still whining it has no printer connected but this has been for a while if i recall correctly. The second serial port is enabled though, which i could not achieve before with MeatPack

Configs.zip

[GhostlyCrowd]

Alright all is working with @rhapsodyv PR. My TFT serial issue was a wiring me and my Kitten need to discuss....

[X-Ryl669]

I'm not sure it's the right way to fix it. You've correctly handled the case with multiserial, but it still have the common "secret" sauce of meatpack.handle_rx_char that's shared between all serial port and as far as I've read, it will mess around when used by both ports (meatpack need more than one char, up to 3 chars sometimes to change its state. So if both port receive a byte successively, meatpack will mix things up here).

I'd say that meatpack should be a per-serial layer so the right fix, IMHO, is to make a per-serial instance of meatpack instead of one per multi-serial and not to store a buffer of parsed status.

[thisiskeithb]

I added a "Needs Discussion" label since there's now two solutions: #21306

[thinkyhead]

Meatpack is only expected on a single serial port — namely, the one that will be permitted to connect to an external host. So I suggest that Meatpack remain limited to a single serial port — both for simplicity and to avoid any extra SRAM usage. If we still prefer to build it out as a single class, then I suggest crafting a template class that takes a "filter class" as one of the template arguments. Most serial ports would use a "NullStreamFilter" but the Meatpack serial port would get a MeatpackStreamFilter class…

… or something like that.

Multi-serial adds no particular complexity re: Meatpack because only the output is "multi" — not the input AFAIK.

[rhapsodyv]

Two things:

1. mks is testing to adopt meatpack support to the cura WiFi plugin to speedup transfer. So, in this case, we may have two serial ports with meat pack enabled (WiFi and usb)

2. I didn’t look at the meatpack parsing code (only the serial code), but if the meatpack parsing code keep internal state (that secret thing), or we make the parser keep a state per serial (easier as my pr) or we make a meatpack parser instance per serial (too much for a simple thing)

I will take a look in the second PR. First I did the MultiSerial<MultiPack<>, MultiPack<>> , but I ended up with a simpler solution. I don’t like the too much the idea of stacks and more stacks of template one on top of each other, and more macros on top of it... if a simpler and correct solution exists.

And the reason is simple: we are build final code here, not a generic api that will be plugged elsewhere. Doesn’t make sense a “generic thing” just to be used “only once in life”...

PS: just waking up now 😜

[thinkyhead]

Just checking the MeatPack code over generally, at the moment the MeatPack class is defined as a singleton, so there is only a single instance. To apply it across multiple Serial port objects, first it should be converted into a regular non-static class, and then any Serial port that needs to be able to handle a MeatPack stream should have its own private object instance of MeatPack. Classes have the option to either include an instance of MeatPack object, or they can just inherit MeatPack as a "mix-in" parent class. As mentioned above, using a common template the default would be to inherit a "null" mix-in class…

[thinkyhead]

Doesn’t make sense a “generic thing” just to be used “only once in life”

Well, in fact it will end up being simpler code if it follows a single pattern, taking a "null filter" by default, and a "meatpack filter" for MeatPack-capable ports, and it only happens as a lucky extra that it makes it easier to plug in similar kinds of serial filters in the future.

[X-Ryl669]

I wonder if Meatpack is transparent by default or not ? I mean, in the protocol, does it require something to be enabled, or it's enabled by default ?

[X-Ryl669]

I've just pushed a non static version of Meatpack in #21306 so it'll not mix things up between instances.

[rhapsodyv]

We can close this and go for #21306 , as it allow the user choose what serial to enable multi pack.

[thinkyhead]

The protocol is fully described in the implementation.

[X-Ryl669]

The protocol is fully described in the implementation.

That's a poor choice. We can't change anything here anymore if the implementation is the protocol.

[X-Ryl669]

To answer the remark above, the code does:

void MeatPack::handle_rx_char_inner(const uint8_t c) {
  if (TEST(state, MPConfig_Bit_Active)) {
[... decode here...]
 else {
   output_char(c)

And MPConfig_Bit_Active is initially not set. So more or less Meatpack is transparent until switched on except for its own command bytes (0xFF) where it's capturing them (2x 0xFF results in one 0xFF emitted).
So we can't really use Meatpack as a transparent filter here as it's not 1:1 transparent.

If we chose to make a dynamic filter (with a pointer to a null filter and switched to Meatpack when enabled), then it means that'll pay the cost of dereferencing the pointer per serial char byte received and the binary space of Meatpack even if we never use it.
I don't know what's good here. The static/compile time approach in #21306 or a dynamic, but a lot simpler, version ?