Skip to content

Fix MeatPack support#20896

Merged
thinkyhead merged 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
scottmudge:bugfix-2.0.x
Jan 27, 2021
Merged

Fix MeatPack support#20896
thinkyhead merged 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
scottmudge:bugfix-2.0.x

Conversation

@scottmudge
Copy link
Contributor

@scottmudge scottmudge commented Jan 27, 2021

Description

This is somewhat urgent for MeatPack, as it is currently broken in the current state.

In order for MeatPack to properly interface with OctoPrint, a space needs to be emitted after the protocol version for the query commands.

I also made the lookup table the default method of decoding, and also made it mutable to ensure that the character library can be dynamic for cases like whitespace removal (replace space character).

The disassembly looked the same for both mutable and const lookup tables, so the performance impact is non-existent.

Requirements

No requirements.

Benefits

It fixes MeatPack.

* Fix MeatPack support.
@CRCinAU
Copy link
Contributor

CRCinAU commented Jan 27, 2021

I'm not sure this fixes the issue right now.... Will converse with the author (hopefully) to troubleshoot...

@CRCinAU
Copy link
Contributor

CRCinAU commented Jan 27, 2021

Adding reference: scottmudge/OctoPrint-MeatPack#10

@CRCinAU
Copy link
Contributor

CRCinAU commented Jan 27, 2021

please read the last half dozen comments in that ticket. It gets a little further, but still doesn't allow two way communication with the printer...

- remove switch/case
- swap characters in lookup table.
@scottmudge
Copy link
Contributor Author

I fixed some other minor problems. Removed the switch/case method, and added mutability to the lookup table for swapping out "E" for " " in whitespace removal mode.

@CRCinAU
Copy link
Contributor

CRCinAU commented Jan 27, 2021

Have given this quite a lot of testing..... Seems to be fine now....

Ok to merge IMHO....

@scottmudge scottmudge changed the title Fix MeatPack support -- needed to add a space. Fix MeatPack support Jan 27, 2021
@thinkyhead thinkyhead merged commit 5e5dfff into MarlinFirmware:bugfix-2.0.x Jan 27, 2021
@thinkyhead
Copy link
Member

Thanks for the patch! Note that the method using logic instead of a lookup table was okay. It considers the no-space flag when returning the character for index 11.

@scottmudge
Copy link
Contributor Author

It looked okay when I gave it a static review, but for some reason it didn't seem to work correctly in practice while debugging with @CRCinAU and @ellensp earlier. I didn't really look into it, but it looked like it should have worked fine in theory.

Earlier when I compiled both approaches for the ATMega2560 and looked at the elf binary under Ghidra, the disassembly was byte-for-byte identical. Regardless of whether the lookup table was const, static, if it was replaced with a switch/case block, etc. The compiler seems to optimize all the arbitrary logic away and used some combination of arrays and pointer arithmetic to address the right character.

But the nice thing about a mutable lookup table is that it allows for a dynamic character library should that feature ever be implemented in the future (e.g., analyzing 16Kbyte blocks before they're sent, and dynamically shifting character-set based on most frequent characters in that block).

Maybe a thought for another day.

Thanks for merging!

@CRCinAU
Copy link
Contributor

CRCinAU commented Jan 27, 2021

Whilst it sounds cool to do dynamic blocks, g-code is a pretty dumb protocol - meaning you won't see exotic variants etc...

I think for the effort required to make sure that all worked, you'd only see fractional improvements in transfer - to the point of it not really making much difference for a ton of added complexity...

Lets see how the release / availability goes now we know it works with all options and reevaluate :)

@scottmudge
Copy link
Contributor Author

Oh it was just a musing. Based on my histogram analysis of about 250 MB of gcode, as you noted, it was 93% the same 15 characters (thus why the table is the way it is, and why this works).

A dynamic library would only really be useful for other character-restricted protocols. Should they still exist.

Jyers pushed a commit to Jyers/Marlin that referenced this pull request Feb 3, 2021
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead pushed a commit that referenced this pull request Apr 30, 2021
Ramis84 pushed a commit to Ramis84/Makerfarm-Pegasus-8-Marlin that referenced this pull request May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants