Skip to content

[2.0.x] Enhanced command injection#9479

Closed
revilor wants to merge 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
revilor:enh_cmd_injection
Closed

[2.0.x] Enhanced command injection#9479
revilor wants to merge 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
revilor:enh_cmd_injection

Conversation

@revilor
Copy link
Contributor

@revilor revilor commented Feb 4, 2018

This PR started by simply implementing an SRAM equivalent of enqueue_and_echo_commands_P in the context of PR #9365.

But a command injected from SRAM could trigger the injection of a command sequence from PROGMEM. This is also a potential problem with the current implementation of enqueue_and_echo_commands_P: if an injected command triggers another command injection the current command sequence would simple be replaced by the new one.

Therefore a stack was introduced to keep track of the injected commands and their respective order. The size of the stack is configurable depending on the complexity of the injected commands. In the context of PR #9365 one could think of a macro calling other macros which would require a larger injection stack.

The feature ENHANCED_COMMAND_INJECTION has to be enabled in Configuration_adv.h and adds enqueue_and_echo_commands_SRAM to the queue interface.

If not enabled the behavious stays as it was: only commands from PROGMEM can be injected using enqueue_and_echo_commands_P and only a single slot for injected commands exists.

@revilor revilor mentioned this pull request Feb 4, 2018
@thinkyhead
Copy link
Member

thinkyhead commented Feb 4, 2018

the current command sequence would simple be replaced by the new one.

Well aware of that, which is why it is generally used for immediate commands where someone clicks a button, and not in normal operation.

It's a good idea, but if it can be done without adding yet another bloody feature (and without requiring a large buffer) that would be preferable.

@ghost
Copy link

ghost commented Feb 11, 2018

Francais ?
J'ai lu l'issue #9365. J'utilise un system singlenozzle que j'ai assemblé, avec volcano interchangeable et un bocal de purge , changer de filament ne me pose pas de probleme , et meme avec mon bocal de purge. Je suis curieux de voir ta machine et ta facon d'interchanger les hotends
Concernant la PR , j'avoue que je suis en mega2512 et donc , avec l'ubl j'ai des stoppies un peu partout des que je pousse le feedrate , mais ca m'interesse quand meme si ca peut rendre mon gcode plus lisible

@revilor
Copy link
Contributor Author

revilor commented Feb 12, 2018

Off topic, but in answer to @studiodyne:

My French is still good enough to read and understand your comment with a little help from Google Translate. But too many years have passed without practice since my French lessons in school, so I have to answer in English:

I added a few pictures to my repository, check https://github.com/revilor/BigTowerDelta/tree/master/pictures/ICH. The hotend modules are mounted to the effector using two screws. Each module carries an NFC tag which stores a label and the nozzle size and also Marlin configuration settings related to the hotend like delta height, PID values, z offset of the probe, or the thermistor used. The effector carries an NFC reader to detect hotend changes and to access the conrfiguration values. My multi-material setup uses Prusa's stepper multiplexer in combination with a self-designed 4x Y splitter and four B2D extruders

@thinkyhead thinkyhead changed the title [FR] Enhanced command injection [2.0.x] Enhanced command injection Feb 16, 2018
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 6 times, most recently from 2308508 to 1e946d6 Compare March 16, 2018 03:23
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from 9f46c52 to 889fd5f Compare March 31, 2018 23:59
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 3 times, most recently from 68147f7 to e8e6026 Compare April 7, 2018 00:48
@marcio-ao
Copy link
Contributor

Well aware of that, which is why it is generally used for immediate commands where someone clicks a button, and not in normal operation.

@thinkyhead : This is a bit of an off-topic rant, but one of my frustrations with Marlin right now is that it is very difficult for a vendor like myself to add new functionality in the middle of an existing GCODE routine, or to implement a new GCODE by calling other GCODEs. Often, this leads to the hack of just dropping a "enqueue_and_echo_commands" in there and crossing one's fingers and hoping it works. Or, worse, setting state variables (i.e. current_position) or using weird functions like planner.buffer_line() or do_blocking_move_xy() without really understanding them and hoping that nothing bad happens.

All this could be avoided if GCODE routines took arguments so that you could call them directly from other places. Currently, they pull from the GCODE parser, so you can't. Just having the ability to call them would be a huge benefit and would cut down on the misuse of enqueue_and_echo_commands.

/end_of_rant

@marcio-ao
Copy link
Contributor

Here's an example of our probe modification, that kind of illustrates the problem:

#define LULZBOT_PROBE_Z_WITH_REWIPE(speed) \
    /* do_probe_move returns true when it fails to hit an endstop, meaning we need to rewipe */ \
    for(int rewipes = 0; do_probe_move(LULZBOT_BED_PROBE_MIN, speed); rewipes++) { \
        if(rewipes >= LULZBOT_NUM_REWIPES) {          /* max of tries */ \
            SERIAL_ERRORLNPGM("PROBE FAIL CLEAN NOZZLE"); /* cura listens for this message specifically */ \
            LCD_MESSAGEPGM(MSG_ERR_PROBING_FAILED);   /* use a more friendly message on the LCD */ \
            LULZBOT_BEEP                              /* play tone */ \
            do_blocking_move_to_z(100, MMM_TO_MMS(Z_PROBE_SPEED_FAST)); /* raise head */ \
            current_position[E_AXIS] = 0;             /* prime nozzle at 75 mm/sec */ \
            planner.buffer_line(current_position[X_AXIS], current_position[Y_AXIS], current_position[Z_AXIS], current_position[E_AXIS], 75./60, active_extruder); \
            sync_plan_position_e(); \
            stepper.synchronize(); \
            kill(PSTR(MSG_ERR_PROBING_FAILED));       /* stop print job */ \
            return NAN;                               /* abort the leveling in progress */ \
        } \
        SERIAL_ERRORLNPGM(MSG_REWIPE); \
        LCD_MESSAGEPGM(MSG_REWIPE); \
        do_blocking_move_to_z(10, MMM_TO_MMS(speed)); /* raise nozzle */ \
        Nozzle::clean(0, 12, 0, 0);                   /* wipe nozzle */ \
    }

Most of this code was figured out by trial and error...

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 3 times, most recently from e0cb8ff to 584735c Compare April 14, 2018 17:28
@thinkyhead
Copy link
Member

Superseded by #10450 which can run G-code sub-procedures immediately, bypassing the queue altogether.

@thinkyhead thinkyhead closed this Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments