gcode: Add serial information for M862.x for P and Q parameters by bkerler · Pull Request #4777 · prusa3d/Prusa-Firmware-Buddy

bkerler · 2025-08-25T22:23:06Z

This partly solves #4157.

When being run remotely, the M862.x P commands should return a response whether it is being compatible or not.
M862.3 P "MK3S" and M862.3 P"MK3S" both should work, but didn't. This behaviour was fixed as well.

CZDanol

Still not a full review, sorry, not much time and this PR is on the bigger side.

Besides from nitpick/comments in the review, there is also a higher-level problem that we're duplicating here what GCodeInfo does. And GCodeInfo does it in a completely different way, so we could end up with a situation where the gcode outputs are telling us one thing and the GCodeInfo is reporting something else.

The way things are implemented in GCodeInfo currently is terrible, because it basically parses the gcodes "by hand". But to allow this extension of the gcodes, we might first need to clean up the GCodeInfo to make things consistent (which would be a big task).

I am not sure.

CZDanol · 2025-09-02T14:17:26Z

src/marlin_stubs/M862_2_3.cpp

+    if (compatibility.is_compatible) {
+        SERIAL_ECHO("Compatibility mode: ");
+        if (compatibility.mk3_compatibility_mode) {
+            SERIAL_ECHOLN("MK3");


I feel like by putting this on separate lines, these will be rather hard to parse by a machine

Agree, changed that.

CZDanol · 2025-09-02T14:17:52Z

src/marlin_stubs/M862_2_3.cpp

+        return;
+    }
+
+    if (p.option<bool>('Q').value_or(false)) {


Nitpick/tip: == true would work as well (but value_or is also fine)

CZDanol · 2025-09-02T14:20:30Z

src/marlin_stubs/M862_2_3.cpp


-    if (parser.boolval('P')) {
-        setup_gcode_compatibility(PrinterModelInfo::from_gcode_check_code(parser.value_int()));
+    if (const auto gcode = p.option<int>('P').value_or(0)) {


Here, value_or is not fine. Technically speaking, 0 could also be a valid printer model. The point of the parser returning std::optional is to avoid these double meanings.

if (const auto gcode = p.option<int>('P')) { print_compatibility(setup_gcode_compatibility(PrinterModelInfo::from_gcode_check_code(*gcode))); }

Changed that as well, good point.

CZDanol · 2025-09-02T14:21:08Z

src/marlin_stubs/M862_2_3.cpp

-
-        setup_gcode_compatibility(PrinterModelInfo::from_id_str(std::string_view(arg, arg_end)));
+    std::array<char, 64> id_str;
+    if (const auto opt = p.option<std::string_view>('P', id_str)) {


Good usage here 👍

CZDanol · 2025-09-02T14:25:49Z

src/marlin_stubs/M862_2_3.cpp

-        setup_gcode_compatibility(PrinterModelInfo::from_id_str(std::string_view(arg, arg_end)));
+    std::array<char, 64> id_str;
+    if (const auto opt = p.option<std::string_view>('P', id_str)) {
+        print_compatibility(setup_gcode_compatibility(PrinterModelInfo::from_id_str(opt->data())));


std::string_view does not guarantee null termination, you cannot just pass the data from it.

from_id_str already takes std::string_view, so there is no need to take the const char* pointer from the view and cast it back to std::string_view. You can just do print_compatibility(setup_gcode_compatibility(PrinterModelInfo::from_id_str(*opt))));

CZDanol · 2025-09-02T14:34:11Z

src/marlin_stubs/M862_6.cpp

+                found = true;
+                break;
+            }
+    #if ENABLED(PRUSA_MMU2)


Yep, sorry, this is way too much code duplication. This MMU3/Phase stepping conditioning should all be implemented on a single place.

Removed for now.

CZDanol · 2025-09-02T14:37:22Z

src/marlin_stubs/M862_6.cpp

+        bool found = false;
+        SERIAL_ECHOPAIR("\"", opt->data(), "\"");
+        for (auto &feature : GCodeInfo::supported_features) {
+            if (opt->find(feature) != std::string_view::npos) {


Find? Find is finding a substring. The match should either be 1:1 or there should be some clearly documented separators.

Removed for now.

CZDanol · 2025-09-02T14:38:19Z

src/marlin_stubs/M862_1.cpp

    }
+
+    if (const std::optional<double> requested_nozzle_diameter = p.option<float>('P')) {
+        if (requested_nozzle_diameter.value() < 0) {


This can be auto-checked by passing a min value to p.option. Also, what is the point of even checking this? If the user enters a negative value, he would get a mismatch anyway.

Done, you're right, negative testing doesn't make sense as we force it to be bigger than 0.1

CZDanol · 2025-09-02T14:41:10Z

src/marlin_stubs/M862_5.cpp

        SERIAL_EOL();
    }
+
+    if (const std::optional<int> req_gcode_level = p.option<int>('P')) {


nitpick: auto would be nicer

CZDanol · 2025-09-02T14:41:55Z

src/marlin_stubs/M862_5.cpp

+
+    if (const std::optional<int> req_gcode_level = p.option<int>('P')) {
+        SERIAL_ECHO("Gcode level ");
+        if (req_gcode_level == (int)GCodeInfo::gcode_level) {


Nitpick: Why cast to int here? Would it be better to take the P option as the same type as gcode_level?

Totally right. Changed that.

bkerler · 2025-09-09T09:57:24Z

I think you are right regarding the redesign and also not happy about the way the mmu3 and feature output/check is happening in M862_6. In order to allow the other fixes to happen, I decided that M862_6 isn't put in this PR, but instead in a separate PR (either with proper changes or after full gcodeinfo patches happened).

bkerler marked this pull request as draft August 25, 2025 23:43

bkerler force-pushed the fix_m862_print branch from 70369de to 3b6daf7 Compare August 26, 2025 11:35

bkerler changed the title ~~gcode: Add serial information for M862.2 and M862.3~~ gcode: Add serial information for M862.x Aug 26, 2025

bkerler marked this pull request as ready for review August 26, 2025 14:50

bkerler force-pushed the fix_m862_print branch from a3ea7f4 to 55acc03 Compare August 26, 2025 14:59

bkerler changed the title ~~gcode: Add serial information for M862.x~~ gcode: Add serial information for M862.x for P and Q parameters Aug 26, 2025

bkerler force-pushed the fix_m862_print branch 2 times, most recently from 26a748c to cc2ffeb Compare August 26, 2025 15:39

bkerler marked this pull request as draft August 26, 2025 15:40

bkerler force-pushed the fix_m862_print branch from cc2ffeb to 2fdd7f0 Compare August 26, 2025 17:02

bkerler marked this pull request as ready for review August 26, 2025 17:03

CZDanol reviewed Sep 2, 2025

View reviewed changes

bkerler force-pushed the fix_m862_print branch 2 times, most recently from 39b9106 to f7583f2 Compare September 9, 2025 09:54

bkerler requested a review from CZDanol September 9, 2025 09:54

danopernis force-pushed the master branch from 77255c9 to b91eeda Compare November 26, 2025 12:42

Bjoern Kerler added 5 commits December 7, 2025 15:38

Add M862.5 P support

6551d28

Add M862.1 P support

16653fb

Add M862.4 P option

06814fd

Add MMU3 to M862.6 P serial output

c470990

Add serial information for M862.2 and M862.3

c5422d5

bkerler force-pushed the fix_m862_print branch from f7583f2 to c5422d5 Compare December 7, 2025 14:41

Conversation

bkerler commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CZDanol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkerler commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bkerler commented Aug 25, 2025 •

edited

Loading