Add experimental support for string registers

[skasti]

Taking inspiration from named ngc-parameters, I have added experimental support for string registers. These can currently only be used to insert the text from the registers into messages to the sender (MSG,, (DEBUG, and (PRINT,, but that is also the use-case I view as most useful for now.

Example use is for storing tool descriptions;

(&1=T1: 4mm 3flute endmill)
(&2=T2: 6mm 45deg chamfer)
(&3=T3: 5.9mm 60deg thread mill)

upon tool change, you can give the user a description of the tool to load:

(MSG: Please load &[#<_selected_tool>] and press 'cycle start')
M1; Wait for user to press cycle start

Or a little more complicated:
P210 (store tool information):

(debug,Store tool info)
#<tool>=#20
#<type>=#17
#<diameter>=#7
#<custom>=#8
(debug,T#<tool> Q#<type> D#<diameter> E#<custom>)
; Start by incementing the index
#<_scnc_info_index>=[#<_scnc_info_index>+1]
(debug,Info-index: #<_scnc_info_index>)
; Then calculate the starting param for the tool info
#<info_offset>=[#<_scnc_info_index>*#<_scnc_info_size>]
#<info_start>=[#<_scnc_info_start>+#<info_offset>]
(debug,Info-start: #<info_start>)
#<_scnc_desc_index>=[#<_scnc_desc_index>+1]
(debug,Description-index: #<_scnc_desc_index>)
#<desc_register>=[#<_scnc_desc_start>+#<_scnc_desc_index>]
(debug,Description-register: #<desc_register>)
G65P3I[#<info_start>]Q[#<tool>]
G65P3I[#<info_start>+1]Q[#<type>]
G65P3I[#<info_start>+2]Q[#<diameter>]
G65P3I[#<info_start>+3]Q[#<custom>]
G65P3I[#<info_start>+4]Q[#<desc_register>]

(&[#<desc_register>]=T#<tool>: L#<length> D#<diameter>)
O60 if [#<custom> NE 0]
    (&[#<desc_register>]=&[#<desc_register>] &[#<_scnc_customtype>+#<type>]#<custom> &[#<type>])
O60 else
    (&[#<desc_register>]=&[#<desc_register>] &[#<type>])
O60 endif

#<_tool_desc>=#<desc_register>

P200.macro (find rack slot for tool):

#<_tool>=#20

G65P201T[#<_tool>]Q[#17]D[#7]E[#8]

G65P111R[#<_tool>]
o70 if [#<_pickupSlot> GE 0]
    #<slot>=[#<_pickupSlot> + 1]
    (MSG,Tool T#<_tool> already stored in slot #<slot> from the back)
    M99
o70 endif

G65P110R[#<_tool>]
o80 if [#<_parkSlot> GE 0]
    #<slot>=[#<_parkSlot> + 1]
    (MSG,Put tool &[#<_tool_desc>] in slot #<slot> from the back)
    M0
    G65P112Q[#<_parkSlot>]R[#<_tool>]
o80 else
    (MSG,Unable to find slot for tool &[#<_tool_desc>]. It will have to be manually loaded/unloaded)
    M0
o80 endif
M99

Then in my programs, I list the tools at the start:

G65P200T1Q4D4E10
G65P200T4Q7D6E0.5

which then results in messages for the operator, so that they understand which tools to load where:
Put tool T1: D4 L10 flat end mill in slot 1 from the back
Put tool T4: D6 CR0.5 bull nose end mill in slot 2 from the back

Updated to reflect how setting string registers is done using comments

[skasti]

Converted to draft as I got some bugs when I want to use parameters in strings 😅

[terjeio]

FYI @ and ^ are used by LinuxCNC for polar coordinates which I plan to add support for later...
I'll take a closer look at this when I am back at base next week.

[skasti]

FYI @ and ^ are used by LinuxCNC for polar coordinates which I plan to add support for later... I'll take a closer look at this when I am back at base next week.

Hah, damnit. I can't very well take @ when it has a well-defined usage already 😅🙈 But it would be very elegant to have a special character for string registers, since using SR or similar makes it easy to mix up with normal text. Will see if some other character might suffice 🤔
Maybe paragraph (§), since that hints at text even more than @ 🤔
Edit; Alas, paragraph is a multi-character character 🤔

[skasti]

Looks like ampesand (&) works, and I did not find any uses for it in grblHAL or gcode that this would interfere with 🤔

[terjeio]

I've been pondering over this while going back and improving the whole parameters and expression implementation...
While doing so I got a new idea for setting string registers, by using comments and trapping them via a grbl.on_gcode_comment event handler.
E.g: (SR[<expr>]=some string)
This is a lot cleaner and solves the issue of block normalization upper casing the string argument. Outputting strings could be achieved in a similar manner?

[terjeio]

Something like this to set string registers:

static void onGcodeComment(char *comment)
{
    int32_t sr;
    uint_fast8_t pos = 3;

    if(!strncmp(comment, "(SR[", 4)) {
        if((ngc_read_integer_value(comment, &pos, &sr)) == Status_OK) {
// check for negative index...
            if(comment[pos] == '=') {
                char c, buf[MAX_SR_LENGTH + 1], *bptr = buf;

                while((c = comment[++pos])) {
                    *bptr++ = c;
                    // perform substitution (expose gcode.c function?)
                }
                *bptr = '\0';
                string_register_set((string_register_id_t)sr, buf);
            }
        }
    }
// return status code
}

void my_plugin_init (void)
{
    grbl.on_gcode_comment = onGcodeComment;
}

grbl.on_gcode_comment should be changed to return a status code, and your & syntax to read values within comments should remain? At least string registers can then live entirely within comments which is, IMO, preferable.

[skasti]

I've been pondering over this while going back and improving the whole parameters and expression implementation... While doing so I got a new idea for setting string registers, by using comments and trapping them via a grbl.on_gcode_comment event handler. E.g: (SR[<expr>]=some string) This is a lot cleaner and solves the issue of block normalization upper casing the string argument. Outputting strings could be achieved in a similar manner?

While it would work, I am not sure I like it. Having comments that are altering semi-persistent stuff feels a bit "wrong", even though I understand the sentiment 😅 And although the current use for string-registers is strictly in comments, I would not be surprised if someone would use them to create engraving plugins etc, and then it would start to look strange to set/modify them only within comments.

As for solving the normalization, I think using quotes will be an elegant solution, as it would then also work if someone at some point adds some other feature that needs to use strings for anything else 🤔 And parameter_substitution can be done in the normalization, which would make that feature available for any feature that needs strings in the future.

But in any case, I would probably keep the syntax for setting and reading similar;

(&10=some text)
(MSG,Here is &10)

or

(SR[10]=some text)
(MSG,Here is SR[10])

not

(SR[10]=some text)
(MSG,Here is &10)

🤔

[skasti]

This belongs in a discussion, but one thing I have thought a little about somehow making a "handler-system" where you would have the default handlers in grbl-core for everything that is core-functionality (supported G-codes and M-codes etc), and plugins could register either customizations of existing handlers or add completely new handlers. That way this could be a plugin that registered a "block-handler" and a "substitution-handler" for & that would handle the setting of and substitution of string registers.

Though I am far away from understanding enough of the code to be able to implement something like this yet other than that it would require a complete rewrite of most of gcode-handling, and is borderline impossible without a test-framework 😅

Edit; Now that I think of it, it might not be too hard to implement such a system for non-letter characters 🤔
Edit2; Forgot to mention that the main point of this would be to clean up gcode.c a lot 😅

[terjeio]

... and then it would start to look strange to set/modify them only within comments.

Strings outside comments in G-code is even stranger, no dialects I know of supports them... The Fanuc SR registers are for robots which is a very specialiced segment? There is also prior use of sending data outside the scope of normal G-code via comments, IIRC LinuxCNC passes some info to its plasma THC implementation that way. And if IIRC that is the reason I added the grbl.on_gcode_comment event (which by the way does not strip the leading ( character as it should - to be fixed...

I agree that gcode.c would benefit from a rewrite, it is an insanely long function. But it is not something that is achieved in a few afternoons, and it works, so better to leave it for later when there is nothing else to improve on?

I have been tinkering a bit more with your string support and want to use it to implement named subroutine calls: o<subname> call ... This by limiting the number of string registers to 65535 (16 bit) and auto-assign an id to named subroutines from max 32 bit value counting down. This way I can pass the subroutine name by id...

A tip: the latest versions supports indexed parameter access with ##n and #[#n] syntax (n can be an expression].

[skasti]

Been busy "playing" carpenter this weekend, so have to try to rebase and add the discussed changes some time during the coming week 👍

[skasti]

@terjeio Trying to move setting string registers to a onGcodeComment-function in string-registers.c causes a circular reference because getting the id of the register to set uses ngc_eval_expression, but ngc_substitute_parameters does string-register substitution and so needs to know about string registers as well 🤔 😅

Maybe substitution of parameters and string registers also should be done using a core handler, like onGcodeComment? 🤔

[skasti]

I have now done some initial testing, and it does seem like this is working. But I have not yet figured out a good way to check for memory leaks. How do you do this, @terjeio ? 🤔

Would be nice if it was possible to get some type of unit testing on the repo as well, so it would be possible to test without deploying to a controller. Not sure how to do that though 😞

[terjeio]

Trying to move setting string registers to a onGcodeComment-function in string-registers.c causes a circular reference because getting the id of the register to set uses ngc_eval_expression, but ngc_substitute_parameters does string-register substitution and so needs to know about string registers as well

True - but you do not need to include the .h file, just declare the needed function, string_register_get(), as extern in ngc_expr.c. The way you have done it now is, IMO, overly complicated.

I am not keen on the new custom status code returns you have added, the old signatures were just fine - just do not output any messages in the string registers code, leave it to the caller if it is needed. BTW string substitution in comments should never cause an error return to the top level - any fault should be handled gracefully. If you insists on having status code returns add them to error.h if there is no suitable codes available there already.

But I have not yet figured out a good way to check for memory leaks. How do you do this

By testing edge cases, and by knowing the behaviour of library calls. Still I miss some sometimes...
In fact you have a potential memory leak that I spotted, if realloc fails (returns null) any previously allocated memory has to be freed in your code.

Would be nice if it was possible to get some type of unit testing on the repo as well, so it would be possible to test without deploying to a controller. Not sure how to do that though

You can use the Simulator driver. I prefer to run tests on a controller, and I make heavy use of the debugger while doing so.

I want to add some functions your string register code in order to support named o subroutines/calls via some trickery (by using 32 bit ids). To do this I want you to use ngc_param_id_t instead of string_register_id_t in your functions, and I'll change string_register_id_t to 32 bit and use it in my "extended" functions. Ok?

[skasti]

By testing edge cases, and by knowing the behaviour of library calls. Still I miss some sometimes... In fact you have a potential memory leak that I spotted, if realloc fails (returns null) any previously allocated memory has to be freed in your code.

Hm, why, though? if realloc returns null, my intention was that the string register just remains unchanged? 🤔

[terjeio]

my intention was that the string register just remains unchanged

Isn't that bad behaviour? I would remove it from the list and return false indicating that setting the register failed.

[skasti]

my intention was that the string register just remains unchanged

Isn't that bad behaviour? I would remove it from the list and return false indicating that setting the register failed.

Not really. Take my example where I append more content on a string register; if appending the extra info fails, I would much prefer to have the base data so that I can try again, rather than losing everything 😅 Regardless; it is not a memory leak when it can still be used/reassigned later, right? 🤔

And if it happens while setting a new SR, it will not have allocated any memory to free, so then it also does not matter either

[skasti]

True - but you do not need to include the .h file, just declare the needed function, string_register_get(), as extern in ngc_expr.c. The way you have done it now is, IMO, overly complicated.

Ah, I forgot that you could do that in C/C++ 😅 While my solution is more complex, it does make string substitution more atomic, where parameter-substitution is completely separated from string register substitution, and others could make plugins that expand it (string substitution) further if they want/need to

[terjeio]

I would much prefer to have the base data so that I can try again, rather than losing everything

I would rather not have som random data coming back later if I decide to reuse a register...
Recently I spent quite a lot of time on a somewhat similar issue with a STM32 processor where memory contents I changed were replaced with the original value after a soft reset due to a processor bug. Not funny...

[skasti]

I would much prefer to have the base data so that I can try again, rather than losing everything

I would rather not have som random data coming back later if I decide to reuse a register... Recently I spent quite a lot of time on a somewhat similar issue with a STM32 processor where memory contents I changed were replaced with the original value after a soft reset due to a processor bug. Not funny...

With all due respect, that is a completely unrelated issue (persisted vs working memory). It is not really that surprising that a change made during runtime is not persisted after a soft reset if there is some issue that causes persistence to fail. Also, it would not be random data, it would be whatever it was before the failed attempt. The error returned should be the hint that something went wrong, not destroying data.

Also, I have not checked, but I am pretty sure that if you change a setting using $x= or a parameter using #x= and it fails, it does not erase the previous value, as that would be very unexpected and destructive behaviour.

Any operation should either have the desired effect, or no effect. This is just best practice in software development.

[skasti]

I am not keen on the new custom status code returns you have added, the old signatures were just fine - just do not output any messages in the string registers code, leave it to the caller if it is needed. BTW string substitution in comments should never cause an error return to the top level - any fault should be handled gracefully. If you insists on having status code returns add them to error.h if there is no suitable codes available there already.

Hm, I agree that string substitution should handle errors more gracefully and not end up returning a error code. And maybe these messages that I posted were mostly not that necessary outside me trying to debug the feature itself 🤔

Though I guess returning an error code for "input too long/large" would be good, so I can add that if none exists already, maybe? 🤔

[skasti]

I want to add some functions your string register code in order to support named o subroutines/calls via some trickery (by using 32 bit ids). To do this I want you to use ngc_param_id_t instead of string_register_id_t in your functions, and I'll change string_register_id_t to 32 bit and use it in my "extended" functions. Ok?

I am not sure I understand why this would require using ngc_param_id_t to fetch string registers? I am guessing you would be using named parameters for the named subroutines, and store the subroutine itself in a string register? if so, would you not store the string register id in the value of the named param? 🤔

[terjeio]

Also, it would not be random data, it would be whatever it was before the failed attempt.

That depends on how you view it. If I run a gcode program that sets a string register and later I run a new one that sets the same register and fails then I would consider it kind of random, or at least totally unrelated, to what I want it to be.

I am not sure I understand why this would require using ngc_param_id_t to fetch string registers?

There are two variants of o call and o sub, numbered where the subroutine is part of the running gcode program and named which is another gcode program located in a filing system where name = the filename of the file. I have already added support for numbered subroutines, to add support for named ones I want to use string registers for temprary storage of the name and use the (automatically assigned) string register id to pass information to ngc_expr.c. The reason for this is primarily that gcode is not designed to handle strings. And since ngc_param_id_t is 16 bit I can differentiate between a "normal" string register and one used by ngc_expr.c for subroutines by assigning ids above 2^16. Some inactive code here and here if you want to take a look.

[skasti]

That depends on how you view it. If I run a gcode program that sets a string register and later I run a new one that sets the same register and fails then I would consider it kind of random, or at least totally unrelated, to what I want it to be.

I would argue that in many scenarios, you would want the value to remain unchanged. IE; you have a string register with the current serial number as a string. every time you run your engraving code, you increment the serial number and regenerate the text. If unable to do so, I would prefer to fetch the register and see what the last successful value was, rather than it being deleted.

In any case; Your gcode program will stop and alert you if there is an error writing to a string register, but it is a lot more hassle to make it repair destroyed data. You would have to make a copy of the register you are about to change first (and a parameter to keep track of the id of the register), and then you have to have a separate program to restore the register that you could run after the program failed 😅 🙈

I would probably use a named parameter to store the string-register id of the subroutine, and then use the string register to look up the file name. no need to mix the Ids? probably missing something here, as I am at work. Will read through the code later and see if I understand then 😅

[skasti]

@terjeio Reading through Calling Files, I get the feeling it would be cleaner to add gc_block.values.o_string as a char*, and add the handler grbl.on_named_macro_execute(char* name) so that it can be implemented the same way as macros? 🤔

I think also I would pass a pointer to gc_block.values to ngc_flowctrl instead of just the O value. A pointer doesn't take much more memory to pass, but allows ngc_flowctrl to use whichever parameters are set in the current block 🤔

That would mean this code;

            } else if(block[char_counter] == '<') {
#if 0
                char o_slabel[NGC_MAX_PARAM_LENGTH + 1];
                if((status = ngc_read_name(block, &char_counter, o_slabel)) != Status_OK)
                    FAIL(status);
                gc_block.words.o = On;
                if((gc_block.values.o = string_register_set_name(o_slabel)) == 0)
                    FAIL(Status_FlowControlOutOfMemory);
                continue;
#else
                FAIL(Status_GcodeUnsupportedCommand); // [For now...]
#endif
            }

could be simplified to this:

            } else if(block[char_counter] == '<') {
#if 0
                if((status = ngc_read_name(block, &char_counter, gc_block.values.o_string)) != Status_OK)
                    FAIL(status);
                gc_block.words.o = On;
                continue;
#else
                FAIL(Status_GcodeUnsupportedCommand); // [For now...]
#endif
            }

And then the handling in ngc_flowctrl could be changed from:

                    if(o_label > NGC_MAX_PARAM_ID) {
#if 0
                        char *subname;
                        if((subname = string_register_get_by_id((string_register_id_t)o_label))) {
                            char filename[60];
                            vfs_file_t *file;

                            strcpy(filename, "/");
                            strcat(filename, subname);
                            strcat(filename, ".macro");

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

                            if((file = vfs_open(filename, "r")) == NULL)
#endif
                            if((file = stream_redirect_read(filename, onError, onFileEnd))) {
                                if((sub = add_sub(o_label, file)) == NULL)
                                    status = Status_FlowControlOutOfMemory;
                            } else
                                status = Status_FlowControlOutOfMemory; // file not found...
                       }
#endif

to something similar to:

                    if(gc_values->o_string != NULL) {
#if 0
                        status_code_t status = grbl.on_named_macro_execute(gc_values->o_string);

                        if(status != Status_Handled)
                            ngc_call_pop();

                        return status == Status_Unhandled ? Status_GcodeValueOutOfRange : (status == Status_Handled ? Status_OK : status);
#endif

And I guess you would also need to free o_string if it is not NULL after processing a block or something 🤔

[terjeio]

I get the feeling it would be cleaner to add gc_block.values.o_string as a char*, and add the handler grbl.on_named_macro_execute(char* name) so that it can be implemented the same way as macros?

Won't work, and having an event is not needed since I do not see any reason why plugin code would (or should) subscribe to it.
BTW ngc_read_name() returns a pointer into the current block so the string goes out of scope when the next block is parsed - the call name cannot then be checked against the sub name called (which is to ensure that the file contains the correct sub). For it to work the string has to be malloc'ed and freed - and extending string register functionality is an easy way to solve it.

I think also I would pass a pointer to gc_block.values to ngc_flowctrl instead of just the O value. A pointer doesn't take much more memory to pass, but allows ngc_flowctrl to use whichever parameters are set in the current block

Of which none is allowed other than N (line number), which is of no use. The rest of the block is to be parsed for expressions to pass on in parameters #1 - #30 (which are to be restored to the previous call level values when the sub returns).

[skasti]

Ok, I see there is much here I still do not understand. I thought this was only going to be used for a single block (O<mysubroutine> CALL), and that this meant it was OK for it to just be viable for that one block 😅🙈
So I will not protest against using string registers for this, though I guess this means I should make a way to delete a string register, so that temporary registers can be cleaned up? 🤔

But I still do not fully understand why you want string-registers to use ngc_param_id_t as id. string-registers are not params 😅
If you want to use the mechanism you mention, it would be "just" as easy to make the string_register_set_name-function return a 32bit uint that is NGC_MAX_PARAM_ID plus the actual string register id, and then subtract NGC_MAX_PARAM_ID from o_label again in ngc_flowctrl?

[terjeio]

I guess this means I should make a way to delete a string register, so that temporary registers can be cleaned up?

I have already coded it, and the other functions I need as well. Works as intended.

string-registers are not params

IMO they are kind of, of string type. But live in a different space. You may keep string_register_t if more logical for you, but keep it as 16 bit, perhaps by typedef'ing it from ngc_param_id_t? I'll add a different type for the 32 bit variant if so.

If you want to use the mechanism you mention, it would be "just" as easy to make the string_register_set_name-function return a 32bit uint that is NGC_MAX_PARAM_ID plus the actual string register id, and then subtract NGC_MAX_PARAM_ID from o_label again in ngc_flowctrl?

I keep an unsigned 32 bit counter for the id's that starts from (uint32_t)-1 (all bits set) and counts down each time a new temp register is created. It would take some time to exhaust that... No need to do any math other than checking for the id beeing > NGC_MAX_PARAM_ID where needed, eg. like this:

void string_register_delete_by_id (string_register_id_t id)
{
    if(id > NGC_MAX_PARAM_ID)
        sr_unlink(id);
}

[skasti]

@terjeio Should handling of (DEBUG, and (PRINT, be moved to ngc_flowctrl's onGcodeComment? 🤔

[terjeio]

No. Since substitution is done in ngc_expr.c it belongs there, and also because the grbl.on_gcode_comment() event is not able to return a string (and should not be changed to accomodate that).

Your grbl.on_string_substitution() event could be set up at startup to point to a new function in ngc_expr.c, making it overridable. And then your string register handling could (or rather should?) be moved to a plugin, in this repo? grbl.on_string_substitution has to be changed to a more descriptive name if doing so...

[skasti]

Aha, maybe I should make a separate PR with just the string substitution-stuff, and move string registers to a plugin-repo? 🤔

[terjeio]

The previous comment was somehow posted before I finished, trying again:

Aha, maybe I should make a separate PR with just the string substitution-stuff, and move string registers to a plugin-repo?

I'll update the core, you add a plugin for string registers and string substitution that includes string registers. This since I'll make comment handling completely overridable even for (MSG, ...), in gcode.c:

            case ')':
                if(comment && !gc_state.skip_blocks) {
                    *s1 = '\0';
                    if(!hal.driver_cap.no_gcode_message_handling) {

                        if(message && *message == NULL) {

                            if(grbl.on_process_gcode_comment)
                                *message = grbl.on_process_gcode_comment(comment);

                            if(*message == NULL) {
                                size_t len = s1 - comment - 3;

                                if(!strncasecmp(comment, "MSG,", 4) && (*message = malloc(len))) {

                                    comment += 4;
                                    while(*comment == ' ') {
                                        comment++;
                                        len--;
                                    }
                                    memcpy(*message, comment, len);
                                }
                            }
                        }
                    }
                    if(*comment && *message == NULL && grbl.on_gcode_comment)
                        *status = grbl.on_gcode_comment(comment);
                }
                comment = NULL;
                break;

grbl.on_process_gcode_comment will be defaulted to handle (PRINT, ..) and (DEBUG, ...) when expressions is enabled.
You can choose to do string replacements only and use the default handler for numeric ones, or replace the handler completely. Note that the grbl.on_process_gcode_comment signature does not allow for a status code return - this is deliberate since message comments should never fail with an error.

You may choose where to publish a string register plugin, you can add it to your own repo or make a PR for the misc plugins repo. The Web Builder can build with repos located outside grblHAL, but local builds cannot without manually adding your code to the source tree - at least not without adding it as a submodule which I am reluctant to do.

FYI I have already added string parameter support to ngc_param.c (not yet comitted) in order to handle named o calls, you may build string registers on top of this or add your version to your plugin. It is different from your string register code in that it does not return status codes and it will remove a string parameter on failure to allocate for it. Also, it will not reallocate on changes if the new length is less than the previous, this as an attempt to reduce heap fragmentation.