Adds macro and DRO functions to I2C keypad

[andrewmarles]

For reference/review, here are the modifications that I made to the I2C keypad plugin to support macros and DRO on the JOG2K. Notably, I am wondering if there is a better way to reference the map_coord_system and disable_lock functions as I had to copy those since they were declared as static. That feels hacky to me so I'm sure there is a better way.

Source code for the Pico on the Jog2K is here:

https://github.com/Expatria-Technologies/I2C_Jog_Controller/blob/main/FW/i2c_responder/app_main.cpp

It is not incredibly sophisticated, but the hardware debounce on the jogger buttons helps to make the firmware simpler.

[terjeio]

AFAIKT these are breaking changes that would require other implementations to be changed. IMO it would be better to split out macro handling and status reporting in separate files and make the functionality optional. Adding keycodes for macros can be done by hooking into keypad.on_keypress_preview, status reporting would require a new function pointer?

Setting_UserDefined_<n> is reserved for user plugins and should not be used here (it is ok in template code), new setting numbers should be reserved for macros.

disable_lock can be called by sending $X to system_execute_line() - be aware of this requirement though: _// NOTE: Code calling system_execute_line() needs to provide a line buffer of at least LINE_BUFFERSIZE

map_coord_system and functions converting coordinates to strings (in report.c) should be made public.

[andrewmarles]

Agree it's disruptive to existing devices. I am probably going to be back working on this again in some detail soon, so I will incorporate your suggestions at that time so that the code is more organized in separate files and the functionality can be made optional. For me, the status reporting really needs a handshake or some kind of exchange that includes a mechanism to indicate the version or type of the data structure so that if they are out of sync it can be reported to the user.

[terjeio]

Here is how I would add macro support to keypad plugin that does not break backwards comaptibility, aux input bound buttons is also supported as an option. MACROS_ENABLE controls the modes, bit 1 for aux buttons, bit 2 for keypad plugin integration. Note that I have not tested the code.

macros.zip

The display interface could be implemented in a similar way? If possible integrated into the core via events?

[terjeio]

Here is an attempt on a separate display plugin along some keypad changes, for discussion:

keypad plugin.zip

A new core event for override changes has been added to the core along with a new keypad event that has a pointer to a struct that contains settings, mode and modifier data as its parameter.

[andrewmarles]

The new macro support in the plugin looks good and I am in the process of merging it in and updating the Jog2K firmware to support it so that there is minimal divergence.

The display code looks also looks pretty logical and straightforward to implement to me.

The only thing that is really different in terms of function from the existing "interactive jog" plugin is the loss of the special spindle macro. I use it for edge-finding so that I can turn the spindle on and off easily, but I think I am the only one that makes use of the function as most people appear to be using 3d probes.

[terjeio]

The only thing that is really different in terms of function from the existing "interactive jog" plugin is the loss of the special spindle macro.

This should be added to the display plugin, or perhaps a custom plugin? I want to keep the macros plugin generic with no hardcoded macros.

The display plugin should be placed in a new directory named display and given a name that is relevant to its use. An id should be assigned to it so it can be enabled with #define DISPLAY_ENABLE in _mymachine.h, similarly to how it is done for VFDs.

[terjeio]

I am not sure what your latest two commits are for...

FYI I have just commited your display code with changes to make it work with the new spindle API++ I stripped down your pendant code and have the display running standalone connected to a Pi Pico - seemingly as it should. I have not tested with the macros and keypad plugins enabled.

I am about to formalize the I2C API by defining the signatures in grbl/plugins.h and will update all drivers and plugins to match - when this is done I will add options for enabling in _mymachine.h, initially for the iMXRT1062 and STM32F4xx drivers.

[andrewmarles]

The issue was that the macro commands were swapped with the Jog2K. And also we found that if you tried to execute an empty macro it would hang the macro parsing function, so I changed the defaults to be G4P0.

[terjeio]

The issue was that the macro commands were swapped with the Jog2K. And also we found that if you tried to execute an empty macro it would hang the macro parsing function, so I changed the defaults to be G4P0

Hardcoded macros should be in the macros plugin? Or in a separate plugin?

FYI I have added a check for the I2C address of the display at startup and if not present it is not enabled. In addition I have added fields for a message at the end of the packet structure, and added a subscription to the new grbl.on_gcode_message event that adds any such message to the packet - making the packet length dynamic. This does not break your Jog2K code, and for testing I modified it and I managed to display messages in the A-axis line...

[andrewmarles]

The message function is really good. I have very much wanted to add something like that (very helpful during toolchanges).

I think that if there are hard-coded macros they should be in a different plugin, I would not expect anything hardcoded to be present in upstream or core code.

In my WIP for the MPG jogging, I added a watchdog to the pendant function that would warn and feed/hold upon loss of connection to the pendant:

https://github.com/Expatria-Technologies/Plugin_I2C_keypad/blob/206dc4c97b7cf19d32e4fe9acd138ae6853fff2d/keypad.c#L412

[andrewmarles]

MPG pendant prototype should arrive soon:

[terjeio]

Yep, loss of connection should be handled. Is hot swapping capability something that should be considered? Or is this already taken care of?

That MPG looks pretty advanced - still using a RP2040?

[andrewmarles]

Yes, the remote_mpg keypad plugin and the I2C buffer that I use for the Jog2K support hot-swapping. The feedhold is there in case the connection is lost by mistake, my intention is to make the hot-swap behavior a config option (choose to do nothing, feedhold or halt). Disconnect is detected via the watchdog trip, and on connection the Jog2K can pulse the KPSTR to alert the host and initiate a handshake. The PB2B715 that is used on Jog2K is 'ok' to be hot-swapped since it has ESD protection and I've not had any issues in my testing.

Yes, the MPG is still RP2040. Expectation is to use PIO state machines for tracking the encoder counts, though an IRQ implementation should be feasible since it is all just manual control. It is really just a glorified version of Jog2K. Still doing macros and the MPG=>jogging vector in the controller.

[terjeio]

FYI I have added some new fields to the machine state struct in gaps inserted by the compiler (due to variable alignment). There are a couple more available.

I also have an idea about adding additional data in the message extension by using the msglen field. When the msglen field is 1-128 the message contains a string, when 255 it is left out altogether indicating the message should be removed from the display. Other values (128-254) could be used to flag additional structures, e.g for responding to settings request etc. This is possible due to the I2C sending a stop when transmission is complete.

I have also played with adding support for the protocol in my MPG & DRO code and I will add it as an option to the communication parser later:

void grblPollI2C (void)
{
    i2c_rxdata_t *i2c_msg; 
    grbl_state_t state;
    machine_status_packet_t *packet;

    if((i2c_msg = i2c_rx_poll()) == NULL)
        return;

    if(i2c_msg->len < offsetof(machine_status_packet_t, msglen))
        return;

    packet = (machine_status_packet_t *)i2c_msg->data;

    switch(packet->machine_state) {

        case MachineState_Alarm:
            state = Alarm;
            break;

//        case MachineState_EStop:
//            state = Alarm;
//            break;

        case MachineState_Cycle:
            state = Run;
            break;

        case MachineState_Hold:
            state = Hold;
            break;

        case MachineState_ToolChange:
            state = Tool;
            break;

        case MachineState_Idle:
            state = Idle;
            break;

        case MachineState_Homing:
            state = Home;
            break;

        case MachineState_Jog:
            state = Jog;
            break;

        default:
            state = Unknown;
            break;
    }

    if(grbl_data.grbl.state != state) {
        grbl_data.changed.state = true;
        grbl_data.grbl.state = state;
        grbl_data.grbl.substate = state == Alarm ? packet->alarm : 0;
        grbl_data.grbl.state_color = grblStateColor[state];
        strcpy(grbl_data.grbl.state_text, grblState[state]);
    }

    if(grbl_data.position[X_AXIS] != packet->x_coordinate) {
        grbl_data.changed.xpos = true;
        grbl_data.position[X_AXIS] = packet->x_coordinate;
    }
    if(grbl_data.position[X_AXIS] != packet->y_coordinate) {
        grbl_data.changed.ypos = true;
        grbl_data.position[Y_AXIS] = packet->y_coordinate;
    }
    if(grbl_data.position[Z_AXIS] != packet->z_coordinate) {
        grbl_data.changed.zpos = true;
        grbl_data.position[Z_AXIS] = packet->z_coordinate;
    }

    if(grbl_data.feed_rate != packet->feed_rate) {
        grbl_data.changed.feed = true;
        grbl_data.feed_rate = packet->feed_rate;
    }

    if(grbl_data.spindle.rpm_actual != (float)packet->spindle_rpm) {
        grbl_data.changed.rpm = true;
        grbl_data.spindle.rpm_actual = (float)packet->spindle_rpm;
    }

    grbl_data.coolant.mist = packet->coolant_state.mist;
    grbl_data.coolant.flood = packet->coolant_state.flood;

    char *pins = spins(packet->signals);
    if((grbl_data.changed.pins = !!strcmp(grbl_data.pins, pins)))
        strcpy(grbl_data.pins, pins);

    if(i2c_msg->len >= offsetof(machine_status_packet_t, msg) && packet->msglen) {
        grbl_data.changed.message = true;
        if(packet->msglen == 255)
            *grbl_data.message = '\0';
        else {
            packet->msg[packet->msglen] = '\0'; // TODO: limit to screen width? Or responsibility of target?
            strcpy(grbl_data.message, packet->msg);
        }
    }

    if(grbl_event.on_report_received && grbl_data.changed.flags)
        grbl_event.on_report_received(grbl_data.block);
}

[andrewmarles]

Closing as this branch is no longer relevant for a PR.