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.

[terjeio]

FYI I have improved the new API and added wrappers/veneers for the old in the latest commit.
It is possible that I will remove the hal.ports interface in a later version.

[engigeer]

I've got a draft working for picomodbus.c, which adds digital + analog inputs and also sets up the queue functions from the Expatria picohal plugin. I've also changed the polling to use the task scheduler, and implemented a basic keepalive (although I am planning to still make some updates so that the keepalive interval can be longer than the basic polling interval).

I've left out all of the non-io functionality (spindle, reacting to state changes, etc.) from this file, and my plan would be to add these messages to the queue from a separate (picohal specific) plugin. Hopefully this aligns with your previous guidance.

Based on the changes you have made with regards to ioexpansion, it seems like it could now make sense for this modbus ioexpansion functionality to be within Plugins_misc alongside the i2c expanders. I tried to follow your advice from earlier and keep it fairly generic. I've done all my testing with the picoHAL, but I see no reason it couldn't work with something like this, provided the addresses are adjusted at compile time.

The naming is still very much WIP, I had initially dropped the HAL to avoid confusion on some of the variable names, but now am thinking that pico is also not the best, since there is no reason that it couldn't work with modbus devices that are not based on rp2040. In either case, curious to know if this is something you would support adding to the upstream, and if there are any particular changes you would want to see first?

[terjeio]

I've taken a closer look at the plugin I wonder why it buffers modbus messages and then send them in blocking mode. Why not just send them non-blocking and thus use the buffering provided by the modbus driver? Is it due to the driver not supporting retries for non-blocking messages?

Since the messages are buffered the ports should be flagged as async as it is now. BTW I think this should be made configurable via the xbar_t config call so that the client code can select either blocking or non-blocking (async) mode. And perhaps blocking mode should be the default?

Another complication is handling pin settings such as signal inversion. This is delegated to the port implementation and is handled by adding a config function like this. And to avoid confusion settings affecting ports should not be duplicated, e.g. inversion of coolant signals should not be available both via $15 (coolant setting) and $372 (aux ouput inversion) when a coolant output is claimed from aux ports. If the port driver supports the set_function call this will be handled by the core by removing the $372 setting flag for the claimed port. A set_function call is easy to add...

it seems like it could now make sense for this modbus ioexpansion functionality to be within Plugins_misc alongside the i2c expanders.

For now I would like it to be a 3rd part plugin both as an example and since it maps to the implementation of the receiving end on the PicoHAL board. A truly generic version would have to query the receiving end for capabilities and configure itself accordingly? And I do not have a PicoHAL board available for testing...
So for devices like the Waveshare linked to a separate plugin is required? Or the plugin would require settings for stuff like addresses to use?

[engigeer]

I've taken a closer look at the plugin I wonder why it buffers modbus messages and then send them in blocking mode. Why not just send them non-blocking and thus use the buffering provided by the modbus driver?

This is more due to my own unfamiliarity with the modbus driver. My starting point was the picohal plugin which implemented its own queue, and I did not realize that this functionality was also available within the modbus driver itself. I will look into this as I would gladly remove the queueing functionality from this plugin to simplify things.

Is it due to the driver not supporting retries for non-blocking messages?

Not exactly (at least I don't think so). This choice was due to some dropped messages I observed on reset if I used non-blocking calls combined with the my custom picohal_spindle(). After I experienced these issues, I took another look at the modbus implementation for VFDs and it seemed that some of the spindle commands were sent as blocking, so I tried changing to blocking and all my issues seem to have resolved. Potentially merits some more investigation, but I didn't find that these "blocking" messages created any issues/stuttering with the motion during some preliminary tests. Is there any reason that blocking would be problematic?

Since the messages are buffered the ports should be flagged as async as it is now. BTW I think this should be made configurable via the xbar_t config call so that the client code can select either blocking or non-blocking (async) mode. And perhaps blocking mode should be the default?

Okay, I will do this. I have also reconfigured the keepalive to always send as non-blocking which I think should be okay.

Another complication is handling pin settings such as signal inversion. This is delegated to the port implementation and is handled by adding a config function like this. And to avoid confusion settings affecting ports should not be duplicated, e.g. inversion of coolant signals should not be available both via $15 (coolant setting) and $372 (aux ouput inversion) when a coolant output is claimed from aux ports. If the port driver supports the set_function call this will be handled by the core by removing the $372 setting flag for the claimed port. A set_function call is easy to add...

I will work on adding this also, I have been meaning to learn more about working with settings and this is a good excuse.

it seems like it could now make sense for this modbus ioexpansion functionality to be within Plugins_misc alongside the i2c expanders.

For now I would like it to be a 3rd part plugin both as an example and since it maps to the implementation of the receiving end on the PicoHAL board. A truly generic version would have to query the receiving end for capabilities and configure itself accordingly? And I do not have a PicoHAL board available for testing... So for devices like the Waveshare linked to a separate plugin is required? Or the plugin would require settings for stuff like addresses to use?

Okay this makes sense. In that case I will leave some of my picoHAL variable naming (at least for now) since I think this makes it a better "example" as it is more clear what would need to be changed to reuse it elsewhere. I guess I should also clarify that I am not affiliated with Expatria, and it is likely that at least some of the WIP changes to the plugin (e.g., switching to the new ioports API) will probably necessitate a new firmware on the receiving end. That said, I have no reason to believe a PR would not be well received, so hopefully a non-issue.

In terms of generality, my plan was to provide compile-time options for the addresses. To interface with the waveshare device the only things that I think would need to change are the modbus device address, and the register addresses for DOUT and AOUT (and of course the number of "pins" for each type). Run-time settings would be another option but, I was thinking that the number of IO ports would likely need to be set at compile-time, and so maybe it makes sense to set everything together rather than splitting things up between run-time and compile-time . . . Perhaps there is a better way to handle this?

[andrewmarles]

The message queue was because back when I first did the picoHAL plug-in I had issues where the vfd rpm would be queried continuously making it impossible to get any other non blocking messages on the bus. With the queue I could have the get_rpm command check only run if at least 30% of the queue was free so that other messages could find space in the line to be sent. I am not sure if all of the other modbus updates since then have resolved that issue.

[engigeer]

Since the messages are buffered the ports should be flagged as async as it is now. BTW I think this should be made configurable via the xbar_t config call so that the client code can select either blocking or non-blocking (async) mode. And perhaps blocking mode should be the default?

It seems that this would require an additional async flag in gpio_out_config_t, or perhaps you had something else in mind?

[engigeer]

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

I have tested using the task scheduler to send the keepalive within my WIP picohal plugin code here. Unfortunately, it seems that it is triggering the same issues (getting caught in a loop) that I had previously observed. I have not been able to reproduce these issues when using the older expatria keypad plugin, so I still think it must somehow relate to the new keypad plugin code. Although, the loop it gets caught in seems to be the keepalive task, so maybe there is something wrong with how I am using the task scheduler in the picohal plugin? Any thoughts / suggestions are much appreciated.

[terjeio]

In terms of generality, my plan was to provide compile-time options for the addresses. To interface with the waveshare device the only things that I think would need to change are the modbus device address, and the register addresses for DOUT and AOUT (and of course the number of "pins" for each type).

Wait with this, get a picoHAL specific version up and running first. Compile time options could be the best but then selected as a single option that sets the parameters so it would be possible to add it to the Web Builder without to much fuss. I would rather avoid adding too many $-settings for this.

It seems that this would require an additional async flag in gpio_out_config_t, or perhaps you had something else in mind?

There and in pin_mode_t as well.

I have tested using the task scheduler to send the keepalive within my WIP picohal plugin code here. Unfortunately, it seems that it is triggering the same issues (getting caught in a loop) that I had previously observed.

Is the receiving end of the picoHAL code/firmware available somewhere so I can set up a test environment on a Pico board? It is hard to tell what might be the issue if I cannot replicate it myself.

[engigeer]

Wait with this, get a picoHAL specific version up and running first. Compile time options could be the best but then selected as a single option that sets the parameters so it would be possible to add it to the Web Builder without to much fuss. I would rather avoid adding too many $-settings for this.

Okay.

Is the receiving end of the picoHAL code/firmware available somewhere so I can set up a test environment on a Pico board? It is hard to tell what might be the issue if I cannot replicate it myself.

Yes, the WIP firmware is here.

[terjeio]

Please download the ModBus spec from here.
The keepalive message should be a diagnostics request, fn = 0x08 with subcode 0x00? See section 6.8. Now it sets the flood output?
When the msb bit is set (checked by 0x80) in the returned function code it is due to an exception at the server side, See section 7.

Also check section 6.21 - this could be used to discover which and the properties of the device connected, I do not know how widely this is implemented, if it is by the e.g. the Waveshare linked above it could be used to configure a generic plugin?

FYI I do get a response to the keepalive message but no luck yet in outputting anything with M64.

I have not been able to reproduce these issues when using the older expatria keypad plugin

Odd that keypad plugin is involved - if not adding that the issue goes away? You run it in I2C mode?

[engigeer]

Please download the ModBus spec from here. The keepalive message should be a diagnostics request, fn = 0x08 with subcode 0x00? See section 6.8. Now it sets the flood output? When the msb bit is set (checked by 0x80) in the returned function code it is due to an exception at the server side, See section 7.

Thank-you for sharing this reference. Unfortunately, I don't think that the python umodbus library has implemented the diagnostic function code. For this initial testing I am just writing to a spare register and using the exception callback to detect if the client is offline. Likely this could be improved, but for now I was just hoping to confirm the task scheduler was working as expected.

Also check section 6.21 - this could be used to discover which and the properties of the device connected, I do not know how widely this is implemented, if it is by the e.g. the Waveshare linked above it could be used to configure a generic plugin?

It seems the umodbus library may not implement this either. Perhaps there are alternatives to umodbus that should be considered for the picohal firmware. I will look into this a bit further.

FYI I do get a response to the keepalive message but no luck yet in outputting anything with M64.

Is it possible you are not using the keepalive_dev branch for the receiver firmware? I have been testing with a slightly stripped down version from the expatria firmware to keep things simple for now. I just checked and the register 0x100 that I am using for keepalive was used by expatria for coolant control so this could explain you are seeing the flood output set.

Odd that keypad plugin is involved - if not adding that the issue goes away?

Yes this strikes me as odd also, but the issue only appears if the keypad plugin is enabled. I have tested successfully with the RP2040 driver without the keypad and with the STM32F4XX driver with the expatria keypad plugin, and in both cases have not experienced this lock-up.

You run it in I2C mode?

Yes. I am also planning to setup a minimal i2C controller implementation with a spare PICO to see if I can reproduce the issue on generic hardware.

Also, while looking through the keypad plugin code, one thing that stuck out to me is that some of the task scheduler calls are within an interrupt context. Wondering if its possible this is creating some sort of race condition if the button is pressed at the wrong time? But it also looks like there are guards for most parts of the task execution with hal.irq.disable, so maybe not an issue . . .

[terjeio]

I don't think that the python umodbus library has implemented the diagnostic function code.

Yep, it does not. Should be easy to add though. Anyway, the keepalive message is, IMO, nasty in that it raises an alarm if it fails. If gcode is running that does not use the expander it might be terminated for no good reason. I would suggest that it should only be sent when in IDLE state. And if you insist it should sent during motion it should only output a warning?

Is it possible you are not using the keepalive_dev branch for the receiver firmware?

I am not, but I modified the python code so it is working now.

Yes this strikes me as odd also, but the issue only appears if the keypad plugin is enabled.

Is the unlock command sent from the keypad? And can you post your $I output so I am sure I have the setup that fails when testing.

FYI I have tweaked your picohal code a bit:

static void picohal_send_message_now (modbus_message_t *data){

    if(!modbus_send(data, &callbacks, true)) {
        if(state_get() != STATE_IDLE)
            system_raise_alarm(Alarm_AbortCycle);
        report_message("PicoHAL communication error.", Message_Warning);
    }
}

Instead of passing the whole message it is faster to pass just a pointer, which is what you use after all.
modbus_send() in blocking mode returns false if it fails due to communication errors. So IMO no need to check for the expander beeing online, it might be up again? And the keepalive message should not interfere with a blocking message, it will just be queued (or ignored - I'll have to check).

static void picohal_send_keepalive (void *data){

    if(!sys.reset_pending)
        modbus_send(&keepalive_msg, &callbacks, false); // should this be non-blocking

    task_add_delayed(picohal_send_keepalive, NULL, PICOHAL_KEEPALIVE_INTERVAL);
}

Here I added a check for reset pending and do not send the keepalive if it is. Perhaps I should add it to the modbus code instead?

In complete_setup() it migh be a good ide to check that the modbus driver is up by calling modbus_isup():

    if(modbus_isup()) {
        on_enumerate_pins = hal.enumerate_pins;
        hal.enumerate_pins = onEnumeratePins;
       ....

Perhaps I should return flags so that one could check explicitly for rtu or tcp...?

I am going to deprecate protocol_enqueue_foreground_task() in the next commit since it should only be used during a cold start of the controller, which generally will be during init functions. I have added a new function, task_run_on_startup(), that should be used instead - and I have aliased protocol_enqueue_foreground_task() to it. Outside of the init function use task_add_immediate() or one of the other add task functions. FYI task_run_on_startup() is guaranteed not to run before the main protocol loop is entered the first time, the others not so.

Also, while looking through the keypad plugin code, one thing that stuck out to me is that some of the task scheduler calls are within an interrupt context.

It should be safe since the task lists are manipulated with IRQ off, but who knows...

[engigeer]

I don't think that the python umodbus library has implemented the diagnostic function code.

Yep, it does not. Should be easy to add though. Anyway, the keepalive message is, IMO, nasty in that it raises an alarm if it fails. If gcode is running that does not use the expander it might be terminated for no good reason. I would suggest that it should only be sent when in IDLE state. And if you insist it should sent during motion it should only output a warning?

I agree the current implementation is a bit aggressive, although this was intentional. If the picohal is used for outputting any critical signals (e.g., laser enable), I think it is important that it falls back to a safe state when there is a loss of communication. My reasoning for the alarm was that if a missed keepalive means the picohal drops to fail-safe state (not yet configured to do so, but in the works), then the controller should be notified with similar urgency to, for example, a loss of comms to the spindle. I see your point that this could be overkill for some uses of the io expansion though. . .

FYI I have tweaked your picohal code a bit:

static void picohal_send_message_now (modbus_message_t *data){

    if(!modbus_send(data, &callbacks, true)) {
        if(state_get() != STATE_IDLE)
            system_raise_alarm(Alarm_AbortCycle);
        report_message("PicoHAL communication error.", Message_Warning);
    }
}

Instead of passing the whole message it is faster to pass just a pointer, which is what you use after all.

Good point, thanks!

modbus_send() in blocking mode returns false if it fails due to communication errors. So IMO no need to check for the expander beeing online, it might be up again? And the keepalive message should not interfere with a blocking message, it will just be queued (or ignored - I'll have to check).

Part of my reasoning for this was to test a modification of the modbus_RTU function so that rx_exception is also called when no response is received for non-blocking messages. Without the is_online guard the keepalive would then spam a new alarm every keep-alive interval . . .

static void picohal_send_keepalive (void *data){

    if(!sys.reset_pending)
        modbus_send(&keepalive_msg, &callbacks, false); // should this be non-blocking

    task_add_delayed(picohal_send_keepalive, NULL, PICOHAL_KEEPALIVE_INTERVAL);
}

Here I added a check for reset pending and do not send the keepalive if it is. Perhaps I should add it to the modbus code instead?

I have also been thinking to implement something that returns all outputs to the off state on reset, maybe with a runtime setting so that it is optional. I wonder if this is a feature that would be useful beyond the picoHAL io?

In complete_setup() it might be a good ide to check that the modbus driver is up by calling modbus_isup():

    if(modbus_isup()) {
        on_enumerate_pins = hal.enumerate_pins;
        hal.enumerate_pins = onEnumeratePins;
       ....

Perhaps I should return flags so that one could check explicitly for rtu or tcp...?

This sounds like a good idea to me.

I am going to deprecate protocol_enqueue_foreground_task() in the next commit since it should only be used during a cold start of the controller, which generally will be during init functions. I have added a new function, task_run_on_startup(), that should be used instead - and I have aliased protocol_enqueue_foreground_task() to it. Outside of the init function use task_add_immediate() or one of the other add task functions. FYI task_run_on_startup() is guaranteed not to run before the main protocol loop is entered the first time, the others not so.

Okay. I will update to the new function once available.

Also, while looking through the keypad plugin code, one thing that stuck out to me is that some of the task scheduler calls are within an interrupt context.

It should be safe since the task lists are manipulated with IRQ off, but who knows...

[engigeer]

Is the unlock command sent from the keypad? And can you post your $I output so I am sure I have the setup that fails when testing.

Yes, I will do this (although I do not have an stm32f4xx board with me over the long weekend) so it will probably be in a few days.

[terjeio]

Update committed, I believe I have resolved the crash issue and also the task lock up. New check for modbus up:

    if(modbus_isup().rtu) {
        on_enumerate_pins = hal.enumerate_pins;
        hal.enumerate_pins = onEnumeratePins;
        ...

[engigeer]

Yes, I will do this (although I do not have an stm32f4xx board with me over the long weekend) so it will probably be in a few days.

Issue seems to be resolved with the latest commit, but just in case this is still helpful, here is the output from $I:

[VER:1.1f.20250419:]
[OPT:VNMSL+,128,1024,4,0]
[AXS:4:XYZA]
[NEWOPT:ENUMS,RT+,NOPROBE,ES,EXPR,SED,ETH,YM,SD]
[FIRMWARE:grblHAL]
[SIGNALS:DE]
[NVS STORAGE:*EEPROM]
[FREE MEMORY:23K]
[DRIVER:STM32F446]
[DRIVER VERSION:250412]
[BOARD:Flexi-HAL]
[AUX IO:6,12,0,2]
[PLUGIN:Bootloader Entry v0.03]
[WIZCHIP:W5500]
[MAC:]
[IP:]
[PLUGIN:FS stream v1.00]
[PLUGIN:FS macro plugin v0.17]
[PLUGIN:MODBUS v0.19]
[PLUGIN:Keypad v1.41]
[PLUGIN:Macros v0.12]
[PLUGIN:I2C Display v0.13]
[PLUGIN:SDCARD v1.23]
[PLUGIN:PicoHAL IOExpansion v0.01]

[terjeio]

Ready for merge now?

[engigeer]

Ready for merge now?

Yes, I have now also added a check for MODBUS_ENABLE, but please feel free to adjust if you would prefer something else.