Macro calling other macros issues

[skasti]

While working on some macros that call other macros to keep code a bit cleaner, I am encountering some issues.

Firstly I am getting a lot of error:39 - Value out of range. when trying to call another macro. Calling the macro in question directly works fine.

Then, after trying to do this 9 times, I start encountering error:82 - Stack overflow while executing flow statement.

Thinking this was strange, as the macro I was testing only calls one other macro, which does not call any more macros, I checked #<_call_level>, and found it to be 9. Based on this, I think that the current call level is not reset when an error is encountered, and thus you will encounter a stack overflow eventually.

I tried a soft-reset, which did not change the call level, so it is not reset properly by resetting the controller Then tried a hard reset, which did reset the call level 😅

After a hard reset, calling my second macro from the first one still does not work (error 39)

P.S; I have tried calling other macros from within my first macro, but the only macro I seem to be able to use, is P1 (the built-in param reader). So I am wondering if there is some validation on the P-word of G65-calls within macros so that is the "value out of range"?

[skasti]

Now I found out that even calling G65P1 corrupts something, so the program fails afterwards. Running this program (P115):

(debug, Clear Tool Rack)
o50 if [#<_scnc_config> NE 1]
    (debug, SCNC Config not initialized. Abort!)
    M0
    M99
o50 endif

#<rack_index> = 0
o60 while [#<rack_index> LT #<_scnc_rack_pockets>]
    (debug, resetting #<rack_index>)
    G65P1Q500
    (debug, ran param read: #<_value>)
    #<rack_index>=[#<rack_index> + 1]
o60 endwhile
(debug, Tool rack slots set to empty)
M99

results in this output:

G65P115
[MSG:Clear Tool Rack]
[MSG:resetting 0]
[MSG:ran param read: 0]
[MSG:Warning: error 2 in macro P115.macro]
error:2 - Missing the expected G-code word value or numeric value format is not valid.

if I then remove the line G65P1Q500 and the following debug, I get this output:

G65P115
[MSG:Clear Tool Rack]
[MSG:resetting 0]
[MSG:ran param read: 0]
[MSG:resetting 1]
[MSG:ran param read: 0]
[MSG:resetting 2]
[MSG:ran param read: 0]
[MSG:resetting 3]
[MSG:ran param read: 0]
[MSG:resetting 4]
[MSG:ran param read: 0]
[MSG:Tool rack slots set to empty]

I will try to do some digging in the source myself as well, to help figure this out, as I really want this to work

[skasti]

Only now realised that macros do not support nesting 🤦

  - Non-Modal Commands: G4, G10L2, G10L20, G28, G30, G28.1, G30.1, G53, G65*****, G92, G92.1
 ...
  ***** requires keypad macros plugin or SD card plugin. Nesting is not allowed.

I wrongly assumed it was supported because of the return-value support etc. My bad! 🤦

[terjeio]

G65P1Q500

This attempts to read setting 500 which may not be available, if not reading it will return an error (_value_returned set to 0).

You may also try replacing M99 with the return statement since this is parsed and executed by the flow control engine.

Only now realised that macros do not support nesting

I have attempted to implement support for nested calls so it may or may not work. Extensive testing/verification is required - I have not invested much time in it yet.

Then, after trying to do this 9 times, I start encountering error:82 - Stack overflow while executing flow statement.

The stacks can be extended here and here. Note that for the first a stack entry is used for the duration of many of the flow control keywords/statements. Increasing the last might allow nested G65 calls.

[skasti]

The stacks can be extended here and here. Note that for the first a stack entry is used for the duration of many of the flow control keywords/statements. Increasing the last might allow nested G65 calls.

Hm, the issue is not that my call stack is 9 levels deep and that I want it deeper. The issue is that when a macro fails due to an error, the "call level" is not reset to 0.

Edit; Ah, MACRO_STACK_DEPTH is what is preventing me from calling a macro from within a macro 😅

Sadly I have not yet gotten to the point where I trust that I am able to build the firmware correctly myself, but once I am, I will try to experiment with this 👍

[skasti]

I have attempted to implement support for nested calls so it may or may not work. Extensive testing/verification is required - I have not invested much time in it yet.

No worries, I will refactor my macros to be one file with subroutines to see if that works for now, and maybe try to get familiar with the codebase so I can be a little helpful 😅

[skasti]

Here is some output from when I reset the controller and run a macro that just prints the current _call_level, which should have returned to 0 after reset;

GrblHAL 1.1f ['$' or '$HELP' for help]
[VER:1.1f.20240604:]
[OPT:VNMHSL,64,1024,4,0]
[AXS:4:XYZA]
[NEWOPT:ENUMS,RT+,HOME,ES,EXPR,SED,ETH,SD,YM]
[FIRMWARE:grblHAL]
[SIGNALS:HSEP]
[NVS STORAGE:*EEPROM]
[FREE MEMORY:26K]
[DRIVER:STM32F446]
[DRIVER VERSION:240428]
[BOARD:FlexiHAL_20240604_060724]
[AUX IO:5,4,0,0]
[PLUGIN:Bootloader Entry v0.02]
[WIZCHIP:W5500]
[MAC:56:34:12:dc:08:00]
[IP:192.168.5.1]
[PLUGIN:MODBUS v0.16]
[PLUGIN:HUANYANG VFD v0.11]
[PLUGIN:HUANYANG P2A VFD v0.10]
[PLUGIN:Durapulse VFD GS20 v0.05]
[PLUGIN:Yalang VFD YL620A v0.02]
[PLUGIN:MODVFD v0.03]
[PLUGIN:H-100 VFD v0.03]
[PLUGIN:Nowforever VFD v0.01]
[PLUGIN:KEYPAD v1.4 Jog2K]
[PLUGIN:Macro plugin v0.02]
[PLUGIN:Probe Protection v0.01]
[PLUGIN:Persistent tool v0.02]
[PLUGIN:SDCARD v1.13]
[PLUGIN:FS macro plugin v0.08]
[SPINDLE:Huanyang v1]
[G54:-189.481,-84.047,-102.291,0.000]
[G55:0.000,0.000,0.000,0.000]
[G56:0.000,0.000,0.000,0.000]
[G57:0.000,0.000,0.000,0.000]
[G58:0.000,0.000,0.000,0.000]
[G59:0.000,0.000,0.000,0.000]
[G59.1:0.000,0.000,0.000,0.000]
[G59.2:0.000,0.000,0.000,0.000]
[G59.3:-334.600,-1.000,-187.000,0.000]
[G28:-184.600,-1.000,-54.000,0.000]
[G30:0.000,0.000,0.000,0.000]
[G92:0.000,0.000,0.000,0.000]
[HOME:0.000,0.000,0.000,0.000:0]
[TLO:0.000,0.000,0.000,0.000]
[PRB:0.000,0.000,0.000,0.000:0]
[VER:1.1f.20240604:]
[OPT:VNMHSL,64,1024,4,0]
[AXS:4:XYZA]
[NEWOPT:ENUMS,RT+,HOME,ES,EXPR,SED,ETH,SD,YM]
[FIRMWARE:grblHAL]
[SIGNALS:HSEP]
[NVS STORAGE:*EEPROM]
[FREE MEMORY:26K]
[DRIVER:STM32F446]
[DRIVER VERSION:240428]
[BOARD:FlexiHAL_20240604_060724]
[AUX IO:5,4,0,0]
[PLUGIN:Bootloader Entry v0.02]
[WIZCHIP:W5500]
[MAC:56:34:12:dc:08:00]
[IP:192.168.5.1]
[PLUGIN:MODBUS v0.16]
[PLUGIN:HUANYANG VFD v0.11]
[PLUGIN:HUANYANG P2A VFD v0.10]
[PLUGIN:Durapulse VFD GS20 v0.05]
[PLUGIN:Yalang VFD YL620A v0.02]
[PLUGIN:MODVFD v0.03]
[PLUGIN:H-100 VFD v0.03]
[PLUGIN:Nowforever VFD v0.01]
[PLUGIN:KEYPAD v1.4 Jog2K]
[PLUGIN:Macro plugin v0.02]
[PLUGIN:Probe Protection v0.01]
[PLUGIN:Persistent tool v0.02]
[PLUGIN:SDCARD v1.13]
[PLUGIN:FS macro plugin v0.08]
[SPINDLE:Huanyang v1]
[G54:-189.481,-84.047,-102.291,0.000]
[G55:0.000,0.000,0.000,0.000]
[G56:0.000,0.000,0.000,0.000]
[G57:0.000,0.000,0.000,0.000]
[G58:0.000,0.000,0.000,0.000]
[G59:0.000,0.000,0.000,0.000]
[G59.1:0.000,0.000,0.000,0.000]
[G59.2:0.000,0.000,0.000,0.000]
[G59.3:-334.600,-1.000,-187.000,0.000]
[G28:-184.600,-1.000,-54.000,0.000]
[G30:0.000,0.000,0.000,0.000]
[G92:0.000,0.000,0.000,0.000]
[HOME:0.000,0.000,0.000,0.000:0]
[TLO:0.000,0.000,0.000,0.000]
[PRB:0.000,0.000,0.000,0.000:0]
G65 P299
[MSG:Call Level: 10]

[terjeio]

I get this with the current version (20240907):

g65p299
[MSG:Call_level: 1]

There has been changes to the codebase since build 20240604 so, IMO, you should update before proceeding.

A side note: calling the inbuilt "macros", P1 and P2, does not need/consume call stack entries.

[skasti]

I have managed to rebuild my firmware with the newest core module now, and I still have some issues with _call_level. However, since I have been rummaging around in the code, I might have found something useful;

From what I can gather, ngc_call_push is always called on a G65-line (gcode.c@L2431), regardless of whether or not a macro is actually started. This means that if you enter the incorrect macro id, or use one of the built-in functions which do not result in calling ngc_call_pop, _call_level will increase by one.

Edit; now that I think of it. maybe the handlers of grbl.on_macro_execute should do the call to ngc_call_push, since the handler of grbl.on_macro_return is the one that calls ngc_call_pop? 🤔

Edit2; Changed the signature of start_macro (in sdcard/macros.c) to return status code, and moved the call to ngc_call_push there. This way, the call_level only increases if a macro is actually started.

[terjeio]

This means that if you enter the incorrect macro id, or use one of the built-in functions which do not result in calling ngc_call_pop, _call_level will increase by one.

Good catch - this is a bug.

Changed the signature of start_macro (in sdcard/macros.c) to return status code, and moved the call to ngc_call_push there. This way, the call_level only increases if a macro is actually started.

Can't do that - ngc_call_push() has to be called before copying G65 parameters since these has to go into a new context. Adding more pop calls a better solution, try this: macros.zip

Note I have not tested this yet, it is bedtime here...

[skasti]

Can't do that - ngc_call_push() has to be called before copying G65 parameters since these has to go into a new context. Adding more pop calls a better solution, try this:

Yeah, that explains what I saw when I tested it. Looked like it worked until I noticed that params weren't populated in the macro 🤦 😅

[skasti]

I think it is a bit complicated that it is up to "all" (in theory multiple at least) handlers of on_macro_execute to know when to call ngc_call_pop(). Just from reading the code you provided, I feel like it would be far too easy for a plugin-dev (like me) to screw this up 😆 Would it be possible to use the returned status code to communicate back to grbl.c that it should do ngc_call_pop()?

example in gcode.c L3528;

                status_code_t status = grbl.on_macro_execute((macro_id_t)gc_block.values.p);
                // If status is not OK (Unhandled or some error), we need to pop the call stack;
                if (status != Status_OK) {
                    ngc_call_pop()
                }
                return status == Status_Unhandled ? Status_GcodeValueOutOfRange : status;

This way, the macro-plugins only need to make sure they return Status_OK if everything is A-OK, otherwise the stack needs to be popped? 🤔

Edit; Ah, but then on_macro_execute would need to return something else than Status_OK when executing internal macros 🤔

[terjeio]

Would it be possible to use the returned status code to communicate back to grbl.c that it should do ngc_call_pop()?

No. Because calling macros is not like calling a subroutine in the normal sense. Calling a macro is basically about temporarily redirecting the input stream to new source (a file on a sdcard or in littlefs), this means the return happens immediately after the stream switch has been done - not when the macro completes. The exception to this is inbuilt macros which does return after the macro code (or rather C code) has been completed.

[skasti]

No. Because calling macros is not like calling a subroutine in the normal sense. Calling a macro is basically about temporarily redirecting the input stream to new source (a file on a sdcard or in littlefs), this means the return happens immediately after the stream switch has been done - not when the macro completes. The exception to this is inbuilt macros which does return after the macro code (or rather C code) has been completed.

which is why it is important to not call ngc_call_pop when returned status is Status_OK, as this would mean that a macro has been started. But if the plugin returns Status_OK when it has executed an internal macro, this would not work, of course.

Naively I would suggest having a new status for Status_Handled (as an internal opposite of Status_Unhandled) that plugins can return to indicate that something is handled internally, and that gcode.c uses a switch to map Status_Unhandled -> return Status_GcodeValueOutOfRange (same as today), Status_Handled -> return Status_OK, and default -> return status

                status_code_t status = grbl.on_macro_execute((macro_id_t)gc_block.values.p);
                // If status is not OK (Unhandled or some error), we need to pop the call stack;
                if (status != Status_OK) {
                    ngc_call_pop()
                }

                switch (status) {
                    case Status_Unhandled:
                        return Status_GcodeValueOutOfRange;
                    case Status_Handled:
                        return Status_OK;
                    default:
                        return status
                }

[terjeio]

Naively I would suggest having a new status for Status_Handled

Good idea, but I would rather return Status_Handled for macro calls that redirects the input stream since they are the odd ones:

#if NGC_PARAMETERS_ENABLE
                if(status != Status_Handled)
                    ngc_call_pop();
#endif
                return status == Status_Unhandled ? Status_GcodeValueOutOfRange : (status == Status_Handled ? Status_OK : status);

and:

static status_code_t macro_execute (macro_id_t macro_id)
{
    status_code_t status = Status_Unhandled;

    if(macro_id < 100) {
        switch(macro_id) { // TODO: add enum or defines?
#if NGC_PARAMETERS_ENABLE
            case 1:
                status = macro_get_setting();
                break;

            case 3:
                status = macro_ngc_parameter_rw();
                break;
#endif
#if N_TOOLS
            case 2:
                status = macro_get_tool_offset();
                break;
#endif
        }
    } else if(stack_idx < (MACRO_STACK_DEPTH - 1) && state_get() == STATE_IDLE) {

        char filename[32];
        vfs_file_t *file;

#if LITTLEFS_ENABLE
        sprintf(filename, "/littlefs/P%d.macro", macro_id);

        if((file = vfs_open(filename, "r")) == NULL)
#endif
        {
            sprintf(filename, "/P%d.macro", macro_id);

            file = vfs_open(filename, "r");
        }

        if(!!file) {
            macro_start(file, macro_id);
            status = Status_Handled;
        }
    }

    return status == Status_Unhandled && on_macro_execute ? on_macro_execute(macro_id) : status;
}

[skasti]

I have been testing this a bit, and it looks promising. But return-values are not working properly for me now, for some reason 🤔

(debug, Find parking)
o50 if [EXISTS[#<_scnc_config>]]
o50 else
    G65P100
    G65P101
o50 endif

#<tool>=#20
#<_index>=0
#<_parkSlot>=-1
o60 WHILE [#<_index> LT #<_scnc_rack_slots>]
    G65P3I[#<_index> + #<_scnc_rack_start>]
    (debug,Slot #<_index>: #<_value> T:#<tool>)
o61 if [#<_value> EQ #<tool>]
        (debug,Returning #<_index>)
        #<_parkSlot>=#<_index>
o60 return #<_index>
o61 elseif [#<_value> EQ -1]
        (debug,Returning #<_index>)
        #<_parkSlot>=#<_index>
o60 return #<_index>
o61 endif
    #<_index> = [#<_index> + 1]
o60 ENDWHILE

This macro looks through my tool rack checking if the tool assigned to a slot is T, or if it is unused (-1), and should return the index of the first matching or available slot. When I run it, I get this;

G65P110T3
[MSG:Find parking]
[MSG:Slot 0: 2 T:3]
[MSG:Slot 1: -1 T:3]
[MSG:Returning 1]

Which indicates that it worked as expected, but when I check _value_returned and _value, I get this;

(MSG,_value_returned: #<_value_returned>)
[MSG:_value_returned: 0]
(MSG,_value: #<_value>)
[MSG:_value: -1]

As you can probably gather from my macro, I have gotten around this by storing my result in _parkSlot, so this is not a big issue for me per se, but it is a bit curious, is it not? 🤔

I suspect it might have something to do with the O60 in front of my returns. I have tried without, which results in an error, and with O61, which I seem to remember yielded the same result 😅

Looking into how the return-values are assigned, maybe it is something simple I have missed, or maybe the language is not really meant to support returning from inside a loop? 😅

[skasti]

Lol, sorry, I was reading through the code now, and finally caught that you have to write your return value of the return statement inside square brackets 🤦