add Picohal plugin init and spindle define

[engigeer]

Request to add plugin init and spindle definition for Expatria picoHAL - modbus RTU connected io-expander and general purpose auxiliary control board.

[terjeio]

Is it for this plugin?

[engigeer]

Yes, this is correct. I am also working on some improvements to submit via a PR. Most notably I have added support for a laser spindle connected to the picoHAL based off of the existing implementation of RS485 VFD spindles. Hence the desire to add a picoHAL spindle definition. Feel free to advise if it is desirable to have this PR accepted by Expatria prior to adding the define, but I wasn’t sure of the proper order to proceed. In either case, hopefully it is possible to add the picohal_plugin_init().

[terjeio]

I would rather have generic names for plugin inits where possible and use id's for enabling a particular one.
E.g. for this one a better name could be ioexpand_enable() and the code guarded by IOEXPAND_ENABLE == 2 where 2 is reserved for picoHAL. I have started to do this for other plugins where multiple implementations may exist.

The spindle part should at least be guarded by SPINDLE_ENABLE and perhaps given a generic name, e.g SPINDLE_PWM3?

The laser specific M-codes should be in another plugin? Possibly with a dependency on picoHAL for now?
Ideally picoHAL should register its ports with the core to make them discoverable by the core for M commands and claimable for plugins. I am slowly working towards making the ioports API capable of handling that seamlessly...

[andrewmarles]

FWIW, I have also been working on a 32 bit I2C expander for an RP2350 based controller. I'm not sure we want to make the selection of I2C based IOEXPAND exclusive with RS485 based expansion.

[engigeer]

I would rather have generic names for plugin inits where possible and use id's for enabling a particular one. E.g. for this one a better name could be ioexpand_enable() and the code guarded by IOEXPAND_ENABLE == 2 where 2 is reserved for picoHAL. I have started to do this for other plugins where multiple implementations may exist.

I agree that something more generic is preferred and also note the point from @andrewmarles that it could make sense to distinguish from onboard ioexpanders. What about something like IOCLIENT_ENABLE, IOREMOTE_ENABLE or IOPERIPH_ENABLE?

The spindle part should at least be guarded by SPINDLE_ENABLE

I have added a code guard within my fork of the picoHAL plugin, following the example of modbus connected VFDs. Is an additional code guard required for the #define within the core? It seems that this would not match the other spindle definitions. . .

and perhaps given a generic name, e.g SPINDLE_PWM3?

I think using PWM3 could be misleading as I was not planning to expose any settings for duty cycle, etc since I think that is best left to the modbus client. Actually, I am not using pwm for my laser but rather TCP/IP requests, although this is somewhat niche and of course shouldn’t be reflected in the name.

How aboutSPINDLE_IOCLIENT or whichever matches the ENABLE above?

The laser specific M-codes should be in another plugin? Possibly with a dependency on picoHAL for now? Ideally picoHAL should register its ports with the core to make them discoverable by the core for M commands and claimable for plugins. I am slowly working towards making the ioports API capable of handling that seamlessly...

Currently my WIP fork for the picoHAL plugin includes both some generic features I plan to submit to the Expatria upstream but also some laser control specific features that I agree should be separated out in their own distinct plugin.

For now I have used custom m-codes to control some additional IO but I am hoping to eventually use the ioports API. I’ve looked at the template code you provided here and it seems it should be relatively straightforward to add ports, but maybe not yet possible to make them available for claiming. I think I would need to claim them if I want to set their state with my custom m-codes - although perhaps there is some other way to do this for now?

[terjeio]

I would rather have generic names for plugin inits where possible and use id's for enabling a particular one. E.g. for this one a better name could be ioexpand_enable() and the code guarded by IOEXPAND_ENABLE == 2 where 2 is reserved for picoHAL. I have started to do this for other plugins where multiple implementations may exist.

I agree that something more generic is preferred and also note the point from @andrewmarles that it could make sense to distinguish from onboard ioexpanders. What about something like IOCLIENT_ENABLE, IOREMOTE_ENABLE or IOPERIPH_ENABLE?

Onboard expanders should go in board specific code, possibly shared between boards? And enabled by IOEXPAND_ENABLE == 1 in combination with the board define if needed? Lately I have started to assign/claim more of the inputs and outputs from the ioport pools and ioexpanders should add to those. New properties for ports such as latency and local vs. external could be added to facilitate selection (by driver and plugin code alike).
The current I2C based expander implementations should be moved to plugin space making them available for all boards supporting I2C? And given their own IOEXPAND_ENABLE id? There is one problem with this and that is how to handle multiple expanders - if that ever becomes an issue...

and perhaps given a generic name, e.g SPINDLE_PWM3?

I think using PWM3 could be misleading as I was not planning to expose any settings for duty cycle, etc since I think that is best left to the modbus client. Actually, I am not using pwm for my laser but rather TCP/IP requests, although this is somewhat niche and of course shouldn’t be reflected in the name.
How aboutSPINDLE_IOCLIENT or whichever matches the ENABLE above?

Looking at the code I only see forwarding of spindle commands (via ModBus) so SPINDLE_GENERIC would be acceptable?

Claiming of ioports is complicated if more than one implementation is behind - since the M62-M68 P and E numbers (IMO) should be consecutive and not having "holes"...

Another detail that, IMO, should be addressed is conversion of controller state to sequential numbers. It would be better if this was standardized, and I want to do this by using ffs(): In v2 of the i2c_interface I do it like this:

static void set_state (sys_state_t state)
{
    status_packet.machine_state = ffs(state);
    status_packet.machine_substate = state_get_substate();

    if(state & (STATE_ESTOP|STATE_ALARM)) {
        char *alarm;

        status_packet.machine_state = SystemState_Alarm;

        if((alarm = (char *)alarms_get_description((alarm_code_t)status_packet.machine_substate))) {
            strncpy((char *)status_packet.msg, alarm, sizeof(status_packet.msg) - 1);
            if((alarm = strchr((char *)status_packet.msg, '.')))
                *(++alarm) = '\0';
            else
                status_packet.msg[sizeof(status_packet.msg) - 1] = '\0';
            msgtype = (msg_type_t)strlen((char *)status_packet.msg);
        }
    }
}

I have added an enum to match the ffs() result in the latest core.

[engigeer]

Onboard expanders should go in board specific code, possibly shared between boards? And enabled by IOEXPAND_ENABLE == 1 in combination with the board define if needed? Lately I have started to assign/claim more of the inputs and outputs from the ioport pools and ioexpanders should add to those. New properties for ports such as latency and local vs. external could be added to facilitate selection (by driver and plugin code alike). The current I2C based expander implementations should be moved to plugin space making them available for all boards supporting I2C? And given their own IOEXPAND_ENABLE id? There is one problem with this and that is how to handle multiple expanders - if that ever becomes an issue...

Okay, if I understand correctly you are suggesting that picohal_init() does not need to be changed here to ioexpand_init() but PICOHAL_ENABLE should be changed to IOEXPAND_ENABLE. This is ok with me, but doesn’t seem to match the other plugins, so just want to confirm.

Looking at the code I only see forwarding of spindle commands (via ModBus) so SPINDLE_GENERIC would be acceptable?

This works for me, although I’m not sure what you mean by only forwarding spindle commands, is there some additional functionality that I could be making use of and am not?

Claiming of ioports is complicated if more than one implementation is behind - since the M62-M68 P and E numbers (IMO) should be consecutive and not having "holes"...

Yes, I can see how this is nontrivial. I will plan to use the existing ioports api functionality without worrying about claiming for now. And I will continue to think about how best to add my laser mcodes on top of this.

Another detail that, IMO, should be addressed is conversion of controller state to sequential numbers. It would be better if this was standardized, and I want to do this by using ffs(): In v2 of the i2c_interface I do it like this:

static void set_state (sys_state_t state)
{
    status_packet.machine_state = ffs(state);
    status_packet.machine_substate = state_get_substate();

    if(state & (STATE_ESTOP|STATE_ALARM)) {
        char *alarm;

        status_packet.machine_state = SystemState_Alarm;

        if((alarm = (char *)alarms_get_description((alarm_code_t)status_packet.machine_substate))) {
            strncpy((char *)status_packet.msg, alarm, sizeof(status_packet.msg) - 1);
            if((alarm = strchr((char *)status_packet.msg, '.')))
                *(++alarm) = '\0';
            else
                status_packet.msg[sizeof(status_packet.msg) - 1] = '\0';
            msgtype = (msg_type_t)strlen((char *)status_packet.msg);
        }
    }
}

I have added an enum to match the ffs() result in the latest core.

Okay, it looks to be straightforward for me to adopt a similar approach to handling states here. BTW, is this what you had referred to in my previous PR on the keypad plugin when you mentioned the need for “status code mapping and using NAN instead of 0xFFFFFFFF for indicating no A axis available”? I have already implemented the NAN support and was planning to submit a PR after I debug one more minor issue.

[terjeio]

Okay, if I understand correctly you are suggesting that picohal_init() does not need to be changed here to ioexpand_init() but PICOHAL_ENABLE should be changed to IOEXPAND_ENABLE. This is ok with me, but doesn’t seem to match the other plugins, so just want to confirm.

I would like to have generic init names - give me some time to think about the best way forward.

Looking at the code I only see forwarding of spindle commands (via ModBus) so SPINDLE_GENERIC would be acceptable?

This works for me, although I’m not sure what you mean by only forwarding spindle commands, is there some additional functionality that I could be making use of and am not?

No, since I cannot see any actual spindle code other than modbus messages then the receiving end actually implements the spindle. And that can be different spindle types depending on what is code running on the other end? There might even be some kind of discovery of capabilities added to the ModBus messaging. So IMO SPINDLE_GENERIC could be an approriate name.

And I will continue to think about how best to add my laser mcodes on top of this.

Most, if not all, of your M-codes could be handled by M62-M68 eqivalents? Is there any official specification of what they do or are they custom codes you have selected? If the latter use of parameter words would reduce the number of M-codes quite a bit.

Okay, it looks to be straightforward for me to adopt a similar approach to handling states here. BTW, is this what you had referred to in my previous PR on the keypad plugin when you mentioned the need for “status code mapping and using NAN instead of 0xFFFFFFFF for indicating no A axis available”?

Yes. We should try to add stuff in a generic and consistent way as possible to avoid confusion and reduce maintenance cost. Hard, I know - but better to spend some time getting stuff right from the start...

[terjeio]

I am working on adding a new middle layer in the core to simplify adding claimable ioports, both from the driver and plugins.
It is nearly complete for final testing, here is how I added digital and "analog" output to picohal:

picohal_ioports.zip

If you or anyone else is willing to test I can provide the changed files (it is only four), for the F4xx driver only for now.

[engigeer]

I am working on adding a new middle layer in the core to simplify adding claimable ioports, both from the driver and plugins. It is nearly complete for final testing, here is how I added digital and "analog" output to picohal:

picohal_ioports.zip

If you or anyone else is willing to test I can provide the changed files (it is only four), for the F4xx driver only for now.

Wow! This looks great. I would absolutely be willing / interested in testing.

[engigeer]

Most, if not all, of your M-codes could be handled by M62-M68 eqivalents? Is there any official specification of what they do or are they custom codes you have selected? If the latter use of parameter words would reduce the number of M-codes quite a bit.

Yes, I think you have a point here. Part of my logic for using the M-codes that I did is past experience working with Fanuc controls so I am familiar with thinking of the M5XX block codes as auxiliary io. I did not necessarily intend for this to be part of the PR to the upstream picoHAL plugin since I recognize that this is likely specific to my use case.

The three main reasons I was leaning towards custom m-codes were:

1. simplifies post-processor creation when dealing with multiple configurations that may or may not have the same io mapping (e.g., if one machine has an extra io port available then the P-values associated with the aux io on the picoHAL may be offset)

2. separate commands to reduce risk of mistyping when entering in MDI (e.g., use M51X block for issuing commands to relays that interact with the laser and M52X block for issuing commands to control air blast, etc.).

3. allows for momentary output actuation without the need for multiple lines of code

After thinking a bit more on this, it seems that perhaps the NGC G65 PXXX macros could be an equivalent solution to create distinct 'alias' for outputs without requiring any custom code. This should address all of my rationale above, but I have limited experience with NGC macros so I am planning to do some testing and see if there is any unexpected latency or other strange behaviour.

Yes. We should try to add stuff in a generic and consistent way as possible to avoid confusion and reduce maintenance cost. Hard, I know - but better to spend some time getting stuff right from the start...

Agreed, and your experience and support with this is much appreciated.

[terjeio]

Here is a version that should work, I added a plugin for the MCP3221 ADC just for reference:

picohal_ioports (2).zip

[engigeer]

Here is a version that should work, I added a plugin for the MCP3221 ADC just for reference:

picohal_ioports (2).zip

Thanks! I had the chance to test the basic functionality today and it seems to work as expected. I will do some more testing (particularly with claiming), but so far so good.

I have also been doing some unrelated testing on error handling for modbus comms issues (mostly trying to ensure that it fails gracefully should a disconnection occur) and I’m wondering if it would be best practice to use the task scheduler to periodically check for reconnection. I’m somewhat hesitant to do so, given this seems to be the potential source of some issues I’ve been troubleshooting with the keypad plugin, but would be curious to know your thoughts on this.

[terjeio]

IMO it is best to use the task scheduler, also for polling. Plugin code should not hook into grbl.on_execute_realtime unless sub millisecond execution is required. And driverReset() should flush the queue?

If the task scheduler is the source of your issue it may be helpful to stress it a bit more?

[terjeio]

I have come up with a separate scheme for registering expander plugins to allow the driver to have the first go at claiming ports.
Expander plugins has to be added to new expanders_init.h core file for this to work.
I have added initial support for board maps using this to the ESP32 and RP2040 drivers, I have to refine this a bit and add some handling if not all required pins can be claimed from ioports. This is an example board map. The pin numbers are the pin numbers declared by the plugin.

Plugins that combines expander code and provides additional functionality should, IMO, register itself as two plugins with separate init functions. One for the expander bit and one for the rest. This to ensure correct sequencing.

I also have added external and async to the capability flags, and a set_function function pointer so that a driver can change the function of the pin to whatever it is used for. Both should be set by plugins. external is for marking the port as external to the MCU and thus not beeing as fast as on chip pins. IMO it should be combined with latency capability, SPI and I2C expanders are usually faster to respond than ModBus or or other similar protocols. async is for ports where the the port output changes some time after the call to change its output returns.

FYI when the driver/board binds to a port it copies the xbar_t struct locally and accesses the port directly via the get_value/set value function pointers provided bypassing the corresponding hal.port calls that will inevitably add overhead. If you want to do the same then note that the struct itself has to be copied since the pointer is pointing to a static copy that is later reused.

[engigeer]

This looks like it will provide a very powerful framework for integrating with the picoHAL. The possibility of claiming external pins for “basic” functions like coolant, spindle, etc., is particularly interesting since it would mean that the picoHAL plugin will not need to hook into all the various core handlers (at least when it is primarily functioning as an ioexpander). I will start to work on refactoring the existing plugin to separate the io functionality as you suggest. Thanks for sharing!

[andrewmarles]

Yeah, I have not dug in detail just yet, hopefully this weekend, but looks perfect for the flexihal2350 which uses i2c for enables, coolant and spindle en/dir.