Add M118 Gcode support #3946
Add M118 Gcode support #3946
Conversation
|
Adding M118 only uses around 58 bytes of flash memory. 👍 Shorter than I expected. It looks like the E and A parameters come from Marlin and not Reprap wiki. |
Firmware/Marlin_main.cpp
Outdated
| } | ||
|
|
||
| if (hasE) SERIAL_ECHO_START; | ||
| if (hasA) SERIAL_ECHO("//"); |
There was a problem hiding this comment.
SERIAL_ECHOPGM("//") ... may be we even have this string already
There was a problem hiding this comment.
Could not find occurence of "//" elsewhere, but changed to use macro SERIAL_ECHOPGM
|
@RoboMagus Have you tested your PR? Sending As MINI firmware is based on Marlin 2.x I would keep the syntax Please try to test PRs and add a short test log to the PRs, this helps a lot to review these. Here my test logs: Here the log when pressing Pause in OctoPrint with some G-Cose scripts |
|
Apologies, I had only tested the code snippets separately as I do not have a 'spare' printer available at the moment to test on. I cannot get the MK404 sim to work, but with the offset added should work now. |
What are your problems with MK404? Are you on Linux? The |
|
I'm using WSL and after working through all missing dependencies and disabling display I cannot get the Serial connection to work. The script immediately exits: |
|
WSL or WSL2? I heard that the new version works better. I used to use WSL and at the end I decided for dual boot and now barely use Windows. |
|
WSL2 indeed, which is better than nothing, but still causes too many LFS / Device related issues for my liking. That is after building with the MK404 build script. I'm fairly confident though that with the latest commit the code in this PR the command should work as expected. Do you have the chance to check again if behavior is now as expected? If not possible I'll hope to get to take some time during this weekend to flash the new FW on printer, as otherwise I could not test this myself. |
|
Had one minor fix left ('M' also still part of Here's a snippet from the Terminal: |
|
@DRracer, After some additional testing with printing files containing This line appears to be the culprit, but I do not understand why a colon ( Edit: I would propose to port these fixes also, as the behavior as it currently stands is not intended. |
|
@RoboMagus thank you for your effort getting your PR tuned and merged. You are right, the However, I'd like to avoid touching the G-code skipping state machine, it took us a lot of effort to get it up to the speed where it is now. That implies that escaping a semicolon |
|
The colon issue should be fixed by PR #3978. This indeed does not add any escape sequence handling to avoid further complication of the GCode reading. I have not yet found any GCode that includes With that little bug squashed the M118 implementation is working as expected on my MK3s at least. |
|
@RoboMagus Sorry for the late review/response. But I'll try look at your PR over the weekend (and test on my printer). 👍 It looks good to me just by looking at the code but I want to test it on the hardware to understand the changes better. |
|
Thanks for the update @gudnimg. I've been running modified firmware on my MK3s for over a month with these changes incorporated (and have been extensively using with a W.I.P. Octoprint plugin) and have not ran into issues yet. Hope your testing yields the same results 😃. |
|
Current memory changes: Flash: +190 bytes I wonder why it needs so much memory 🤔 |
|
I tried my best to make M118 fail but these results look good me. I think we should try to reduce the memory usage if we can. Note the timestamps below are just from Arduino IDE, not from firmware. I noticed M118 will skip spaces in the beginning of the message:
Not sure if this is expected behavior. Tests: Input Input Input Input Input Input: Input |
|
Our new CMake builds also give us the .asm output. Here it is for this PR just to share it: case 118: {
bool hasE = false;
bool hasA = false;
char *p = strchr_pointer + 5;
1a54e: 00 91 4c 03 lds r16, 0x034C ; 0x80034c <strchr_pointer>
1a552: 10 91 4d 03 lds r17, 0x034D ; 0x80034d <strchr_pointer+0x1>
1a556: 0b 5f subi r16, 0xFB ; 251
1a558: 1f 4f sbci r17, 0xFF ; 255
1a55a: 83 e0 ldi r24, 0x03 ; 3
- `E1` - Prepend echo: to the message. Some hosts will display echo messages differently when preceded by echo:.
- `String` - Message string. If omitted, a blank line will be sent.
*/
case 118: {
bool hasE = false;
bool hasA = false;
1a55c: f1 2c mov r15, r1
- `A1` - Prepend // to denote a comment or action command. Hosts like OctoPrint can interpret such commands to perform special actions. See your host’s documentation.
- `E1` - Prepend echo: to the message. Some hosts will display echo messages differently when preceded by echo:.
- `String` - Message string. If omitted, a blank line will be sent.
*/
case 118: {
bool hasE = false;
1a55e: 40 e0 ldi r20, 0x00 ; 0
1a560: 81 50 subi r24, 0x01 ; 1
bool hasA = false;
char *p = strchr_pointer + 5;
for (uint8_t i = 2; i--;) {
1a562: e9 f0 breq .+58 ; 0x1a59e <process_commands() [clone .part.41]+0x3d7e>
// A1, E1, and Pn are always parsed out
if (!((p[0] == 'A' || p[0] == 'E') && p[1] == '1')) break;
1a564: d8 01 movw r26, r16
1a566: 9c 91 ld r25, X
1a568: 29 2f mov r18, r25
1a56a: 2b 7f andi r18, 0xFB ; 251
1a56c: 21 34 cpi r18, 0x41 ; 65
1a56e: b9 f4 brne .+46 ; 0x1a59e <process_commands() [clone .part.41]+0x3d7e>
1a570: 11 96 adiw r26, 0x01 ; 1
1a572: 2c 91 ld r18, X
1a574: 21 33 cpi r18, 0x31 ; 49
1a576: 99 f4 brne .+38 ; 0x1a59e <process_commands() [clone .part.41]+0x3d7e>
switch (p[0]) {
1a578: 91 34 cpi r25, 0x41 ; 65
1a57a: 71 f0 breq .+28 ; 0x1a598 <process_commands() [clone .part.41]+0x3d78>
1a57c: 95 34 cpi r25, 0x45 ; 69
1a57e: 09 f4 brne .+2 ; 0x1a582 <process_commands() [clone .part.41]+0x3d62>
case 'A': hasA = true; break;
case 'E': hasE = true; break;
1a580: 41 e0 ldi r20, 0x01 ; 1
}
p += 2;
1a582: 98 01 movw r18, r16
1a584: 2e 5f subi r18, 0xFE ; 254
1a586: 3f 4f sbci r19, 0xFF ; 255
1a588: 89 01 movw r16, r18
1a58a: 2f 5f subi r18, 0xFF ; 255
1a58c: 3f 4f sbci r19, 0xFF ; 255
while (*p == ' ') ++p;
1a58e: f8 01 movw r30, r16
1a590: 90 81 ld r25, Z
1a592: 90 32 cpi r25, 0x20 ; 32
1a594: c9 f3 breq .-14 ; 0x1a588 <process_commands() [clone .part.41]+0x3d68>
1a596: e4 cf rjmp .-56 ; 0x1a560 <process_commands() [clone .part.41]+0x3d40>
for (uint8_t i = 2; i--;) {
// A1, E1, and Pn are always parsed out
if (!((p[0] == 'A' || p[0] == 'E') && p[1] == '1')) break;
switch (p[0]) {
case 'A': hasA = true; break;
1a598: ff 24 eor r15, r15
1a59a: f3 94 inc r15
1a59c: f2 cf rjmp .-28 ; 0x1a582 <process_commands() [clone .part.41]+0x3d62>
}
p += 2;
while (*p == ' ') ++p;
}
if (hasE) SERIAL_ECHO_START;
1a59e: 44 23 and r20, r20
1a5a0: 21 f0 breq .+8 ; 0x1a5aa <process_commands() [clone .part.41]+0x3d8a>
1a5a2: 8c ef ldi r24, 0xFC ; 252
1a5a4: 9b ea ldi r25, 0xAB ; 171
1a5a6: 0e 94 f6 89 call 0x113ec ; 0x113ec <serialprintPGM(char const*)>
if (hasA) SERIAL_ECHOPGM("//");
1a5aa: ff 20 and r15, r15
1a5ac: 21 f0 breq .+8 ; 0x1a5b6 <process_commands() [clone .part.41]+0x3d96>
1a5ae: 86 e2 ldi r24, 0x26 ; 38
1a5b0: 96 e8 ldi r25, 0x86 ; 134
1a5b2: 0e 94 f6 89 call 0x113ec ; 0x113ec <serialprintPGM(char const*)>
SERIAL_ECHOLN(p);
1a5b6: c8 01 movw r24, r16
1a5b8: 0e 94 5a 94 call 0x128b4 ; 0x128b4 <MarlinSerial::println(char const*)>
1a5bc: 0c 94 6e c4 jmp 0x188dc ; 0x188dc <process_commands() [clone .part.41]+0x20bc> |
How do you find the increase in mem usage?
All I can think of that's been changed in commits since then has been to use |
This behaviour is copied from the Marlin implementation to ensure behaviour does not deviate between implementations. I cannot think of a usecase where spaces would be usefull to retain myself, but if there would be one it'd be best in my opinion to keep implementation consistent rather than deviate based edge usecases. |
When I build the firmware via Vscode + CMake I get this result: Keep in mind the number for the memory in Flash (label Program in the image) doesn't include the bootloader which eats up 8192 bytes of the free memory. As you can see we don't have that much memory free which is why I'm constantly looking for optimisations so we can squeeze more into the firmware 😅 I usually build the latest current MK3 branch, take a screenshot of the result. Then switch to the branch I'm working on and use my screenshot to compare with the result on my branch. Note I am applying your changes to the latest MK3 branch, maybe something changed since you opened the PR which is increasing the code size but I don't see how. I have found this alone takes 70B? hmm I've also found one potential optimisation which may help in this PR. And that's to create a common function to skip whitespace such that:
|
|
I'm seeing this only is adding 70B 🤔 Strange the compiler doesn't optimize this away. |
|
It looks like the memory difference is due to the compiler or linker optimizing differently. So the issue is not your PR :) Probably some function is being inlined somewhere by LTO (not sure how to find it though, the asm file is huge). |
|
@DRracer / @3d-gussner |
There was a problem hiding this comment.
Now with some more space we can finally merge this PR.
The proposed changes are scope-limited.
Future improvements may include:
- extracting M118 implementation away from Marlin_main.cpp (which may help the compiler to optimize the code better)
- adding unit tests to see how malformed M118 records manage to break the whole thing
|
@RoboMagus Thank you for keeping your PR up to date and responding to our reviews. |
A long standing Feature Request, ported
M118support from Marlin.Related issues: