MPG & DRO: two independent data streams?

[nickshl]

As I understand current version of MPG & DRO can have two input streams and one output stream. Both sender and MPG controller will see responds on each other requests. Also current realization need to switch main device(whatever it means) either using external pin(in some cases no available pins) or with 0x8B command which required Keypad plugin(not an elegant solution to enable plugin just for this command).

Another option is to make two independent streams: multiple senders should not see each other responses. In many cases there no problems with this approach. The only one limitation that I see: if one of sender start executing program, grblHAL should reject commands from another sender that will affect current program(with error code that allow to figure out that other sender executes program), but still should accept some commands that can be send by user itself(like reset, pause, cycle start, may be jog command in Hold state if it is allowed). When program execution ends grblHAL can accept commands from both senders again.

I started work on pendant with MPG and capacitive touch screen, ideally(that the plan) it will be act as fully functional sender with ability to stream program from SD card. For now I have to build two versions: USB and UART and reprogram controller to switch between interfaces.

[terjeio]

... or with 0x8B command which required Keypad plugin(not an elegant solution to enable plugin just for this command).

I do not think I will add functionality for opening multiple streams with switchover to the core so it is likely a plugin will be required. It does not need to be the "standard" keypad plugin that handles stream switchover - any plugin can potentially do that.

What would likely be possible to add to the core without breaking stuff is passing a stream handle (pointer to the stream struct) to all reporting functions so that a plugin can direct output to its own stream.

The only one limitation that I see: if one of sender start executing program, grblHAL should reject commands from another sender that will affect current program(with error code that allow to figure out that other sender executes program)

This is the hard part - how to implement this in a backwards compatible way (with senders)?

[nickshl]

This is the hard part - how to implement this in a backwards compatible way (with senders)?

It is already should be backward compatible: any sender should be able to handle errors just fine. Program can have unsupported command and grbl will throw an error. Sender can try to send jog command during run cycle and grbl will throw an error.

[terjeio]

Until I get around to add stream handle to commands that write to a given stream you can wrap function calls that outputs data with claim and restore of the hal.stream.write function:

io_stream_t my_stream;

void foo_write (void)
{
    stream_write_ptr write = hal.stream.write;
    hal.stream.write = my_stream.write;
    call_output_function(); // e.g. report_gcode_modes()
    hal.stream.write = write;
}

void my_plugin_init (void)
{
    io_stream_t *stream;

    if((stream = stream_open_instance(STREAM_ID, 115200, NULL))) {
        memcpy(&my_stream, stream, sizeof(io_stream_t ));
        foo_write();
    }
}

Note that a few calls writes to all registered streams via hal.stream.write.all, these are:

report_alarm_message
report_init_message
report_realtime_status

It is already should be backward compatible: any sender should be able to handle errors just fine.

The main issue is, IMO, how to detect that a connected client is streaming commands? The closest is perhaps to check the gc_state.file_run boolean - but this is dependent on the sender delimiting the job by the % character.

[nickshl]

The main issue is, IMO, how to detect that a connected client is streaming commands?

You mean how to detect that one of sender started executing program to reject commands from other senders? When one of senders start execute program, state changing to "Cycle". At this point grblHAL should return error to G-code commands from other senders. When state change back to "Idle", grblHAL can accept commands from other senders again. For "Jog" state grblHAL an accept and execute commands from all senders - if user wants to crank MPG on pendant by one hand and hit jog button on PC by another hand, that is totally fine.

[terjeio]

When one of senders start execute program, state changing to "Cycle".

This is not neccesarily true. If the planner buffer is starved you may se "Idle" state intermittently, and also when a sync is executing - e.g. during a G4 command. Anyway, please try your ideas out - you can do a lot of stuff within the current framework.

[nickshl]

I have a question:

Mode switching via the 0x8B real time command character requires the keypad plugin enabled

Why it required keypad plugin? It looks like embedded grblHAL functionality(since it can be switched via dedicated pin), why it can't be handled by grblHAL itself like any other single character command?

[terjeio]

Why it required keypad plugin?

It just evolved that way. If the core is to provide support for stream switching then it has to monitor the stream input itself, and then another question arises: how many, if any at all, of the other real time commands should be supported by the core. I can add a new MPG mode (or modes?) that enables this, but would need input on whether other (or which) real-time commands should be handled. FYI when switching via a dedicated pin no real-time commands are processed when MPG mode is disabled - unless the keypad plugin is added to the build.

[nickshl]

I have an another "brilliant" idea! Thanks to design flaw of BlackPill and STM bootloader. What if instead dedicated pin we will use RX line on grblHAL controller? If this line pulled high - it mean other device(like pendant) enabled UART and want to send something. And if this line low, it mean that nothing is connected. Pendant can pull UART TX line low when it want to give control to the sender. The only problem is transmission, since it pulled like low we can't just look level for disconnect, we should look for low interval long enough to be sure that it is not part of transmission.

It may be useful when no dedicated pin available, like in this "ideal" pinout: https://github.com/grblHAL/STM32F4xx/blob/master/Inc/blackpill_alt2_map.h (by the way, my nick it typed incorrectly 😃). Sure, I can use one of unused encoder input, or A limit input, or even Stepper En/Dis(useless signal when microstepping is used), but if this pin can be combined with UART RX line - this is ideal solution.

That how STM bootloader on BlackPill works - when bootloader detect high level on UART RX pin, it switch to load via UART mode instead of USB. And when I left this pin floating, bootloader picked up noise - I wasn't able to load firmware at all until I pulled down UART RX pin with external 10k resistor.

[terjeio]

I have an another "brilliant" idea! ...

This is a breaking change that will require changes to both existing MPG implementations and possibly all drivers supporting MPG mode. Feel free to try it out, if you can somehow implement this in a plugin that would be nice.

[nickshl]

This is a breaking change that will require changes to both existing MPG implementations and possibly all drivers supporting MPG mode.

Technically my proposal is almost the same as the existing solution and can be implemented easily. STM32 can use UART RX and can read pin state at the same time(probably other MCUs can too). For grblHAL there is no difference to check the state of the dedicated pin or UART RX pin. So, if dedicated pin set as control pin - grblHAL uses it, if UART RX pin set as control pin - grblHAL uses it. There is no difference whatsoever. The only change needed is to check low level long enough - simplest solution to check that all N sequential reads of a pin return low(polarity also can be configurable via config file). And the driver should be aware of that and if the control pin sets the same as the UART RX pin, it should be configured as UART RX, not input pin.

In this case all existing MPG implementations that uses dedicated pin will be unaffected.

Anyway, could you send me config with a secondary UART configured controlled via a dedicated pin based on this config: https://github.com/grblHAL/STM32F4xx/blob/master/Inc/blackpill_alt2_map.h ? UART pin is already defined, you can use Limit A as a dedicated pin. I will use it as a starting point to investigate it further.

[terjeio]

Anyway, could you send me config ...

What do you mean by that? A modified map file?

[nickshl]

Yes. Modified blackpill_alt2_map.h file that enables USB as primary communication and UART with Limit A as a hardware control pin for MPG communication. Just attach it there - I'll build it myself.

[terjeio]

Add this to the map:

#if MPG_MODE == 1
#define MPG_MODE_PORT           GPIOB
#define MPG_MODE_PIN            15
#endif

USB is enabled in _mymachine.h and is default on for all boards except for the Nucleo based ones.

[nickshl]

I did it. All previous functionality does not affected. I tried it with my MPG and it works just fine. The only thing is at startup MPG is show 0, but after first send $J= command it show MPG as 1. It probably happens because MPG shouldn't request control some time after startup. I think I can fix it on MPG side by pulling UART TX line low and high again to retry gain control Full code(including .git folder) if there: STM32F4xx.zip Changes pretty much self-explanatory. Please review it and if it looks ok to you, add it to repo(except my_machine.h obviously). If you have any questions or concerns, please let me know.

[nickshl]

Hi @terjeio, have you had a chance to review those changes?

[terjeio]

Not yet, I am away for some time and do not have access to hardware for testing.

[nickshl]

Any updates on this issue?

Basically this change should not affect any current configuration and can be safely be added. I tested those a little bit and it works just fine - ioSender lock controls when I request control from my MPG.

Core code has change in this line: https://github.com/grblHAL/core/blob/2c58f0de090a36628400d25d0109a066655c507f/pin_bits_masks.h#L210 to #if !defined(MPG_MODE_PIN) || defined(MPG_MODE_POOLING) // We don't need interrupt in pooling mode Without define MPG_MODE_POOLING this code works exactly the same, and define MPG_MODE_POOLING is new, no existing configuration that contain it.

Driver code for STM32F4xx has more changes:

Since MPG mode pin shared with UART, it should not be initialized. So, MPG_MODE_NOINIT define added and next code: https://github.com/grblHAL/STM32F4xx/blob/5f601d9fe16087ce65b62b2f2dcc791dadebeeea/Src/driver.c#L199 should be changed to: #if defined(MPG_MODE_PIN) && !defined(MPG_MODE_NOINIT) Same deal with this line: https://github.com/grblHAL/STM32F4xx/blob/5f601d9fe16087ce65b62b2f2dcc791dadebeeea/Src/driver.c#L2562 It should be changed to: #if (MPG_MODE == 1) && !defined(MPG_MODE_NOINIT)

Current code want to see 0 as control request. Since idle state for UART is 1, and 1 should indicate control request. To preserve compatibility with current code, define MPG_POLARITY is added which will defaulted to 0 if it isn't defined. Next code: https://github.com/grblHAL/STM32F4xx/blob/5f601d9fe16087ce65b62b2f2dcc791dadebeeea/Src/driver.c#L1613C1-L1626C7 should be changed to:

#if MPG_MODE == 1

#ifndef MPG_POLARITY
#define MPG_POLARITY 0
#endif

static void mpg_select (sys_state_t state)
{
    stream_mpg_enable(DIGITAL_IN(MPG_MODE_PORT, MPG_MODE_PIN) == MPG_POLARITY);
}

static void mpg_enable (sys_state_t state)
{
    if(sys.mpg_mode != (DIGITAL_IN(MPG_MODE_PORT, MPG_MODE_PIN) == MPG_POLARITY))
        stream_mpg_enable(true);
}

#endif

And a last piece of code should be added to Driver_IncTick() function:

#if (MPG_MODE == 1) && defined(MPG_MODE_POOLING)
    static bool     mpg_input_state = false;
    static uint32_t mpg_counter = 0u;

    // Check if pin state isn't changed
    if(mpg_input_state == (DIGITAL_IN(MPG_MODE_PORT, MPG_MODE_PIN) == MPG_POLARITY))
    {
        // Check 100 times
        if(mpg_counter < 100u) mpg_counter++;
        // On 101 time
        else if(mpg_counter == 100u)
        {
            // Try to switch mode
            protocol_enqueue_rt_command(mpg_select);
            // And increase counter to prevent entering this statement again
            mpg_counter++;
        }
        else 
        {
            ; // Do nothing 
        }
    }
    else // If pin state have changed
    {
        // Save new pin state
        mpg_input_state = (DIGITAL_IN(MPG_MODE_PORT, MPG_MODE_PIN) == MPG_POLARITY);
        // And clear the counter
        mpg_counter = 0u;
    }
#endif

If state of UART RX line changed and next 100 sequential reads(which takes100 ms) returns the same state - it mean that mode should be changed.

One more thing I forgot to mention: my board has pull-down resistor on RX line, which mean no MPG request if nothing is connected to UART. This resistor needed anyway because without it BlackPill not always enter in DFU by pressing BOOT button(bootloader messed up when see high level on UART RX), but it may be not present. Just in case it would be nice to enable pull-down resistor on UART RX line in case if board doesn't have that and MPG control pin is UART RX.

[terjeio]

have you had a chance to review those changes?

It would be better if you link to a forked repo with the changes or create a PR for review. This way I can see the changes without applying the changes to a fork that I have to create myself. Anyway, from the code and comments you have provided in your last comment I am of the opinion that the changes needed should either be implemented as a driver specific plugin or as board specific code. This since it seem to me that the functionality cannot be universally applied, see below.

Just in case it would be nice to enable pull-down resistor on UART RX line in case if board doesn't have that and MPG control pin is UART RX.

You mean enable the internal pull down or an external one? When I add an external 4K7 pulldown to my Nucleo board the RX pin stays high even when nothing is connected to it.

[nickshl]

I am of the opinion that the changes needed should either be implemented as a driver specific plugin

There not a lot of changes to make it plugin. And at this point it pretty driver specific(for STM32F4 only), one line in core have to be changed only to avoid Interrupt initialization for UART RX line.

This since it seem to me that the functionality cannot be universally applied, see below.

Should it be universal? I thought that #defines is for - to build version exactly match with hardware and needs.

When I add an external 4K7 pulldown to my Nucleo board the RX pin stays high even when nothing is connected to it.

It would stay high on Nucleo board if UART is PA2 & PA3. You have to remove SB13 or SB14, I don't remember which one: By default it connected to UART on ST-Link part of Nucleo, and TX from ST-Link pulls RX on Nucleo high. Even 1k(that what I use in my controller) pull-down resistor on controller RX line doesn't communication at all.

You mean enable the internal pull down or an external one?

Internal one. But it is not a big deal - it easy enough to add resistor or build version without MPG support.

[terjeio]

MPG RX is on PA10. And I was testing with a FTDI breakout - this pulls PA10 high even with a pulldown resistor if the USB cable is not inserted.

one line in core have to be changed only to avoid Interrupt initialization for UART RX line.

If you add a setting for enabling MPG mode in the plugin/board specific code then there is no need to change the core, driver code or the Web Builder.

[nickshl]

MPG RX is on PA10. And I was testing with a FTDI breakout - this pulls PA10 high even with a pulldown resistor if the USB cable is not inserted.

You have to disconnect FTDI after you pull-down the RX line on controller. Idea that MPG MCU will reconfigure UART TX as output and set it low when it would like to give up control. When it want to gain control again, it will reconfigure UART TX as UART TX and it will pull line high. Pull-down resistor I am talking about needed only in case when nothing connected to UART and RX line is floating. In BlackPill case this line have to be pulled down anyway to avoid "Device not recognized" error on PC when you connect it with BOOT button pressed.

If you add a setting for enabling MPG mode in the plugin/board specific code then there is no need to change the core, driver code

Change in core needed only to ability not enable interrupt for MPG line. And I don't understand why core messed up with interrupt at all. Isn't it is a job of driver? (it is rhetorical question) Anyway, why I did multiple define instead one like MPG_MODE_UARD_RX_LINE_CONTROL? Because I want it to be universal. Interrupt for MPG line works fine... until it doesn't. STM32 interrupts is limited. You can not use pins on different ports with same number as interrupt(i.e. PA5 and PB5) - it will not work. If you already using interrupt for something on one port, you can not use MPG control signal on same pin number on different port - that is define MPG_MODE_POOLING for - to have control without interrupt. MPG_POLARITY - allow to change polarity of control signal. If you want by some reason to be high level to gain control instead low - that is this define MPG_POLARITY for. And only MPG_MODE_NOINIT somewhat directly related to control via UART because it tells that something else initialized control pin(in my case it is UART code). All those defines can be used independently for different reasons.

or the Web Builder.

Web Builder doesn't need any change if MPG_MODE 1 can be forced in map file.

[terjeio]

You have to disconnect FTDI after you pull-down the RX line on controller.

Yes I understand that - not very practical though and potentially unsafe if the pins are not protected, easier just to unplug the USB cable. Or better just route the RTS or DTR signal to the MPG input and switch mode when the pendant app open/closes the port.

And I don't understand why core messed up with interrupt at all. Isn't it is a job of driver? (it is rhetorical question)

There is no executable code in the core that I am aware of that does that other than via the HAL. There are signal definitions and preprocessor code related to signalling in the core - placed there in order to ensure that all drivers uses a common naming scheme.

[terjeio]

Here is example board specific code that does what you want with no changes (exept a "bug" fix) to existing code:

#include "driver.h"

#if defined(BOARD_BLACKPILL_ALT2)

static xbar_t rx_pin;
static on_execute_realtime_ptr on_execute_realtime, on_execute_delay;

static void uart_rx_poll (void)
{
    static bool     mpg_input_state = false;
    static uint32_t mpg_counter = 0u, last_ms = 0;

    uint32_t ms;

    if(last_ms != (ms = hal.get_elapsed_ticks())) {

        last_ms = ms;

        // Check if pin state isn't changed
        if(mpg_input_state == (DIGITAL_IN((GPIO_TypeDef *)rx_pin.port, rx_pin.pin) == 1))
        {
            // Check 100 times
            if(mpg_counter < 100u) mpg_counter++;
            // On 101 time
            else if(mpg_counter == 100u)
            {
                // Try to switch mode
                stream_mpg_enable(mpg_input_state);
                // And increase counter to prevent entering this statement again
                mpg_counter++;
            }
            else
            {
                ; // Do nothing
            }
        }
        else // If pin state have changed
        {
            // Save new pin state
            mpg_input_state = DIGITAL_IN((GPIO_TypeDef *)rx_pin.port, rx_pin.pin) == 1;
            // And clear the counter
            mpg_counter = 0u;
        }
    }
}

static void modbus_poll_realtime (sys_state_t grbl_state)
{
    on_execute_realtime(grbl_state);

    uart_rx_poll();
}

static void modbus_poll_delay (sys_state_t grbl_state)
{
    on_execute_delay(grbl_state);

    uart_rx_poll();
}

static void get_rx_pin (xbar_t *pin, void *data)
{
    if(pin->function == Input_RX && pin->group == (PinGroup_UART + (uint8_t)((uint32_t)data)))
        memcpy(&rx_pin, pin, sizeof(xbar_t));
}

static bool claim_stream (io_stream_properties_t const *sstream)
{
    if(sstream->type == StreamType_Serial && !sstream->flags.claimed) {
        if((hal.driver_cap.mpg_mode = stream_mpg_register(stream_open_instance(sstream->instance, 115200, NULL), false, NULL)))
            hal.enumerate_pins(true, get_rx_pin, (void *)((uint32_t)sstream->instance));
    }

    return hal.driver_cap.mpg_mode;
}

void board_init (void)
{
    if(!hal.driver_cap.mpg_mode && stream_enumerate_streams(claim_stream) && rx_pin.port) {

        on_execute_realtime = grbl.on_execute_realtime;
        grbl.on_execute_realtime = modbus_poll_realtime;

        on_execute_delay = grbl.on_execute_delay;
        grbl.on_execute_delay = modbus_poll_delay;
    }
}

#endif

I added #define HAS_BOARD_INIT to the board map and fixed the "bug" in driver.h:

#define DIGITAL_IN(port, pin) BITBAND_PERI((port)->IDR, pin)

[terjeio]

FYI the xbar_t struct has an entry for get_value that could be used to read the RX pin status. If assigned in driver.c then the above code can be modified to become a generic plugin. Only a few lines of code has to be added in driver.c for that. E.g:

static bool get_gpio_input (struct xbar *pin)
{
    return DIGITAL_IN((GPIO_TypeDef *)pin.port, rx_pin.pin);
}

...
    if(ppin) do {
        pin.pin = ppin->pin.pin;
        pin.function = ppin->pin.function;
        pin.group = ppin->pin.group;
        pin.port = low_level ? ppin->pin.port : (void *)port2char(ppin->pin.port);
        pin.mode = ppin->pin.mode;
        pin.description = ppin->pin.description;

        if(pin.function == Input_RX && pin.group >= PinGroup_UART && pin.group <= PinGroup_UART4)
            pin.get_data = get_gpio_input;

        pin_info(&pin, data);

        ppin = ppin->next;
    } while(ppin);
}
...

This line above: if(mpg_input_state == (DIGITAL_IN((GPIO_TypeDef *)rx_pin.port, rx_pin.pin) == 1)) can then be changed to: if(mpg_input_state == rx_pin.get_data(&rx_pin) == 1)) and voila, the code becomes generic and can be used as a plugin with all drivers that has _rx_pin.getdata assigned and supports reading UART RX pin status.

Note that I have not tested this so it may not compile...

[nickshl]

This solution seems to work. I added pull request based on that: https://github.com/grblHAL/STM32F4xx/pull/151

         if(pin.function == Input_RX && pin.group >= PinGroup_UART && pin.group <= PinGroup_UART4)
            pin.get_data = get_gpio_input;

Dou you mean get_value(_pin doesn't have getdata)? What is the point to have "_pin.group >= PinGroup_UART && pin.group <= PinGroupUART4" in this statement? Isn't any Input_RX should be able to be read with get_value function? By the way, why it has float argument?

And one more thing: I don't like this code. I bet any(probably) static analyzer will have complains about that. It should be something like this:

         if((pin.function == Input_RX) && (pin.group >= PinGroup_UART) && (pin.group <= PinGroup_UART4)) {
            pin.get_data = get_gpio_input;
         }

I personally also doesn't like when open braces isn't aligned with closing braces(it doesn't allow to find start of the block from one sight), but it is just personal preferences...

         if((pin.function == Input_RX) && (pin.group >= PinGroup_UART) && (pin.group <= PinGroup_UART4))
         {
            pin.get_data = get_gpio_input;
         }

[terjeio]

Dou you mean get_value(pin doesn't have get_data)?

The get_data function pointer is not assigned per default - because I have not referenced it in existing code. It was added a while back for future use.

Isn't any Input_RX should be able to be read with get_value function?

Yes, you could argue that. Reading a pin is likely harmless so the test could be simplified.

By the way, why it has float argument?

Ouch, I missed that. It is to allow reading analog inputs.

And one more thing: I don't like this code.

We have different preferences then.

[nickshl]

We have different preferences then.

Preferences - it is where to place code block braces: same line or next line. Braces for piece of code in if statement it is not a preferences. Sure, you can cross street outside of pedestrian crossing and call it is "preferences". But you will increase risk. Same in software development. That is why standards like MISRA exists and why static analyzers use it and want you to comply with them.

[terjeio]

Braces for piece of code in if statement it is not a preferences.

The compiler accepts it. If you do not like it you can fork the code and rewrite it to be MISRA compliant or to whatever else style guide you choose.

I have my style and I try to stick to that - and I am not going to change it or enter into any argument about it.