Suggestion: Always trigger State change on boot/reset

[5ocworkshop]

I am working on status lights for my machine and if the machine comes up in STATE_IDLE, it does not appear to fire the onStateChanged event.

However, if it comes up in STATE_ALARM (homing required) it does fire. It would be good to fire a state change during boot and reset events, so anything chained to that trigger has a chance to perform functions at boot/reset.

[5ocworkshop]

It might even be useful to have a STATE_BOOT or STATE_RESET flag (or both?).

[terjeio]

However, if it comes up in STATE_ALARM (homing required) it does fire. It would be good to fire a state change during boot and reset events, so anything chained to that trigger has a chance to perform functions at boot/reset.

I'll see if I can add this without too many changes. You may also handle this locally in your plugin, see below.

It might even be useful to have a STATE_BOOT or STATE_RESET flag (or both?).

These events are already trappable, the plugin init function is called once during boot and you can subscribe to reset events by adding a function to the hal.driver_reset chain. There is code in the ioports template showing how to add a reset handler.

[5ocworkshop]

These events are already trappable, the plugin init function is called once during boot and you can subscribe to reset events by adding a function to the hal.driver_reset chain. There is code in the ioports template showing how to add a reset handler.

Thanks, I'll check that out.

Another thought about entry points. I'm still getting familiar with the code but I want to be able to flash the light for certain states, like ALARM. To do this in a non-blocking way I need to loop, but with event driven call backs I can't be assured that I'll be called in a reliable interval.

For now I switched the flashing code in to the Report update callback, but that only works when the sender is connected and reliably asking for updates. Granted, this is the expected action and most senders comply, but the intervals vary. And the current stable/official release of CNCjs stops querying during ALARM states (which is likely not the intended/desired situation, but it is what it is) which interferes with the loop.

Would it be possible to have an entry point at the end of the main loop that is triggered not by an event but by the the end of the loop being reached in addition to the more classical trigger events already present?

[terjeio]

Would it be possible to have an entry point at the end of the main loop that is triggered not by an event but by the the end of the loop being reached in addition to the more classical trigger events already present?

I have a couple of examples in the pipeline, one is a blinky:

/*

  my_plugin.c - user defined plugin that blinks the LED on a STM32F411 Blackpill

  Part of grblHAL

  Public domain

*/

#include "driver.h"

static on_report_options_ptr on_report_options;
static on_execute_realtime_ptr on_execute_realtime;

// Add info about our plugin to the $I report.
static void on_report_my_options (bool newopt)
{
    on_report_options(newopt);

    if(!newopt)
        hal.stream.write("[PLUGIN:Blink LED v1.00]" ASCII_EOL);
}

static void blink_led (sys_state_t state)
{
    static bool led_on = false;
    static uint32_t ms = 0;

    if(hal.get_elapsed_ticks() >= ms) {
        ms = hal.get_elapsed_ticks() + 500; //ms
        led_on = !led_on;
        if(led_on)
            GPIOC->ODR |= GPIO_PIN_13;
        else
            GPIOC->ODR &= ~GPIO_PIN_13;
    }

    on_execute_realtime(state);
}

void my_plugin_init (void)
{
    // Add info about our plugin to the $I report.
    on_report_options = grbl.on_report_options;
    grbl.on_report_options = on_report_my_options;

    // Add blink LED function to grblHAL foreground process
    on_execute_realtime = grbl.on_execute_realtime;
    grbl.on_execute_realtime = blink_led;

    // Enable PC13 as output
    GPIO_InitTypeDef GPIO_InitStructure = {
        .Mode      = GPIO_MODE_OUTPUT_PP,
        .Speed     = GPIO_SPEED_FREQ_VERY_HIGH,
        .Pin       = GPIO_PIN_13,
    };
    HAL_GPIO_Init(GPIOC, &GPIO_InitStructure);
}

[5ocworkshop]

Excellent, this is exactly what I was looking for. Is this available today or in your "to be committed in the near future" stack?

[5ocworkshop]

Here is what I am doing for now for the flashing portion.

Is there a HAL call that will return the current alarm sub-code? I haven't been able to find one yet.

Also, is there an example available of reading the spindle state via the hal.spindle.get_state call? Spindle on is one of the key things I want track.

I've not tried to optimize the code yet (and I'm a relative novice), still trying to work out my core logic.

I am hoping to be able to do things like: for a limit switch triggered alarm, flash red but add a fast pulse of an axis colour to the sequence, so you get a pulse of blue for Z, green for Y and red for X sensors. I've just got to figure out how to extract those details from the hal calls.

---

void rgb_set_state (reqcolor) {
    hal.port.digital_out(red_port, RGB_MASKS[reqcolor].R);
    hal.port.digital_out(green_port, RGB_MASKS[reqcolor].G);
    hal.port.digital_out(blue_port, RGB_MASKS[reqcolor].B);    
}

static void onRealtimeReport (stream_write_ptr stream_write, report_tracking_flags_t report)
{
    static sys_state_t local_state;  // For STATE_* 
    static alarm_code_t alarm_code; // For Alarm_*  Only present it STATE_ALARM is asserted?

    currentMS = hal.get_elapsed_ticks();

    local_state = state_get();

    // Alarm states
    if ((local_state == STATE_ALARM || local_state == STATE_ESTOP) && rgb_flash_state == 0 && ( currentMS - startMS >= RGB_FAST)) {
    //if (alarm_code == Alarm_HardLimit && rgb_flash_state == 0 && ( currentMS - startMS >= RGB_FAST)) {
        rgb_set_state(RGB_RED);
        rgb_flash_state = 1;
        startMS = hal.get_elapsed_ticks();
    }
    if ((local_state == STATE_ALARM || local_state == STATE_ESTOP) && rgb_flash_state == 1 && ( currentMS - startMS >= RGB_FAST)) {
        ///if ((alarm_code == Alarm_HardLimit) && rgb_flash_state == 1 && ( currentMS - startMS >= RGB_FAST)) {
        rgb_flash_state = 0;
        rgb_set_state(RGB_OFF);
        startMS = hal.get_elapsed_ticks();
    }

    // Other events with that involve flashing lights - used to indicate immediate human attention required
    // Door ajar, Hold, Tool Change etc.

    if ((local_state == STATE_HOLD ) && rgb_flash_state == 0 && ( currentMS - startMS >= RGB_FAST)) {
        rgb_set_state(RGB_YELLOW);
        rgb_flash_state = 1;
        startMS = hal.get_elapsed_ticks();
    }
    if ((local_state == STATE_HOLD) && rgb_flash_state == 1 && ( currentMS - startMS >= RGB_SLOW)) {
        rgb_flash_state = 0;
        rgb_set_state(RGB_OFF);
        startMS = hal.get_elapsed_ticks();
    }

    if(on_realtime_report)
        on_realtime_report(stream_write, report);
}

[5ocworkshop]

Your example works great. I'm going to move my code over to that callback.

[5ocworkshop]

Also, is there an example available of reading the spindle state via the hal.spindle.get_state call? Spindle on is one of the key things I want track.

Found this too:

(!(hal.spindle.get_state().on))

[terjeio]

Is this available today or in your "to be committed in the near future" stack?

More examples were comitted yesterday.

Is there a HAL call that will return the current alarm sub-code? I haven't been able to find one yet.

There is no call, you can read the alarm code from sys.alarm, the contetnt is only valid in the alarm state.

Also, is there an example available of reading the spindle state via the hal.spindle.get_state call? Spindle on is one of the key things I want track.

You can find one in grbl/report.c:

https://github.com/grblHAL/core/blob/2068165e62c949bda37e7006aebd1406b18d76d2/report.c#L1156

https://github.com/grblHAL/core/blob/2068165e62c949bda37e7006aebd1406b18d76d2/report.c#L1249-L1251

You can also chain your code into the HAL entry point so you do not need to poll. See the plasma plugin for how to do that. Note that some function pointers are reset on settings changes, hal.spindle.set_state is one - you have to trap setting changes and re-chain.

I've just got to figure out how to extract those details from the hal calls.

Chaining in your own code into the HAL is in a way the same as subscribing to events, you can do all kinds of fancy stuff with that. Check out the code in the provided plugins for ideas...

Your example works great. I'm going to move my code over to that callback.

I've not tried to optimize the code yet (and I'm a relative novice), still trying to work out my core logic.

A tip: polling for everything in the grbl.on_execute_realtime event handler is not the best approach, IMO it is better subscribe to events (or chain into HAL calls) and process them. More code but less overhead.

[5ocworkshop]

Thank you for the excellent feedback and all your work and deep thought on this project. The more I learn about how you have laid it out the more impressed I am with the structure and extensibility. Great job!

[5ocworkshop]

Question regarding:

if(state != last_state) {
        last_state = state;
        hal.port.digital_out(port, state == STATE_HOLD);
    }

Is that an elegant way of saying "Only execute the hal.port.digital_out(port) call if the state is == STATE_HOLD? If so, could you have multiple of these in a row and they would almost act like a chain of if statements or a case statement? I hadn't realized you could put a test inside the function call, that is very helpful.

[terjeio]

Is that an elegant way of saying "Only execute the hal.port.digital_out(port) call if the state is == STATE_HOLD?

No - it is a way to turn the output on or off with a single call on a state change. You can also write the hal.port.digital_out() call like this:

if(state == STATE_HOLD)
    hal.port.digital_out(port, true);
else
    hal.port.digital_out(port, false);

or like this: hal.port.digital_out(port, state == STATE_HOLD ? true : false);

The last one is useful when the parameter value is something else than a boolean.

If so, could you have multiple of these in a row and they would almost act like a chain of if statements or a case statement?

It depends, it will result in a bit of overhead since the function call will always be executed.

I hadn't realized you could put a test inside the function call, that is very helpful.

You can even use a function call as a parameter as long as it returns the correct type. No need to use a temp variable. An example: memcpy(&hal.stream, serialInit(), sizeof(io_stream_t));

[terjeio]

I forgot this one you mentioned above:

if(!hal.spindle.get_state().on)
...

This can be written like this (most will do so?):

spindle_state_t state = hal.spindle.get_state();
if(!state.on)
...

This is neat trick if only one element in a struct/union return value needs to be referenced. Again, no need for the temp variable.

[5ocworkshop]

This is neat trick if only one element in a struct/union return value needs to be referenced. Again, no need for the temp variable.

That is a very neat trick. These are all great details.

All the tips are very much appreciated. I'm going to annotate my plugin extensively. I'd love to get your thoughts/code review of it when I'm ready to share.