Move modbus code to separate folder?

[grblhal]

Move modbus code to separate folder?

[terjeio]

The code can be moved to a separate submodule, but the code has to be changed a bit in order to support more than one client as there are two callbacks that is bound when the Huanyang plugin is initialized. I guess these has to be per transaction instead, to be added to the queue_entry_t struct?

[dresco]

I wonder, could they be chained, and then each client could just ignore if not for it's own modbus slave address? (the rx_exception callback would also need the msg passed back to it for context).

[terjeio]

I wonder, could they be chained

They could, but IMO adding the callbacks to each message is cleaner - and I can get rid of the public modbus_stream_t struct. That would allow access to the modbus code from custom my_plugin.c implementations.

Here is my suggestion (I have not tested the code):

spindle.zip

What do you think?

[dresco]

Thanks, will try and test that tomorrow..

[dresco]

Hi, afraid is not working just yet..

Is faulting - I think because packet is set to NULL by the ModBus_AwaitReply case in modbus_poll(), before the ModBus_Timeout and ModBus_Exception cases are reached in modbus_send(). Checking that packet is non null removes the faults, but then the timeouts and exceptions never actually do anything useful.

I then tried commenting out the packet = NULL in ModBus_AwaitReply instead, as it seems this is done after the callbacks are processed in modbus_send() anyway - but I must be missing something else in the logic, as the regular messages from spindleGetState() just stop..

Cheers, Jon.

[dresco]

Actually, this seems to work (I had been overlooking the different code paths for sync and async packets)..

$ git diff
diff --git a/modbus.c b/modbus.c
index a8c355e..3724a3e 100644
--- a/modbus.c
+++ b/modbus.c
@@ -172,14 +172,15 @@ void modbus_poll (sys_state_t grbl_state)
 
         case ModBus_AwaitReply:
             if(rx_timeout && --rx_timeout == 0) {
-                if(packet->async)
+                if(packet->async) {
                     state = ModBus_Silent;
+                    packet = NULL;
+                }
                 else if(stream.read() == 1 && (stream.read() & 0x80)) {
                     exception_code = stream.read();
                     state = ModBus_Exception;
                 } else
                     state = ModBus_Timeout;
-                packet = NULL;
                 spin_lock = false;
                 if(state != ModBus_AwaitReply)
                     silence_until = ms + silence_timeout;

[terjeio]

Do you think this is an ok way to implement support for multiple clients then?

[dresco]

I only got as far as testing with the Huanyang plugin yesterday. I'll port my proof of concept control panel plugin across today, and try them both together..

[dresco]

Hmm, have them both working independently, but no luck if enabled together.. Thinking perhaps the modbus & serial initialisations should just be called once from driver.c?

#if SPINDLE_HUANYANG > 0
    huanyang_init(modbus_init(serial2Init(115200), NULL));
#endif

#if PANEL_ENABLE
    panel_init(modbus_init(serial2Init(115200), NULL));
#endif

[terjeio]

Thinking perhaps the modbus & serial initialisations should just be called once from driver.c?

Sure - I'll split the modbus_init() to a separate call. Your panel_init() should be without any parameters.

[dresco]

Thanks, can confirm it is working well with two clients now.

Have noticed that the Huanyang VFD appears to intermittently stop responding with a silence of less than ~6ms, leading to timeouts/spindle alarms. My test environment is running at 19200 baud with cheap auto switching RS485 modules, however my modbus slave device (STM32 BlackPill board with FreeMODBUS stack) is reliable at the default 2ms silence, so will blame the Huanyang implementation for now. :)

Also uncovered a small bug while troubleshooting the above - the configured silence timeout was not always respected, due to a 16/32 bit comparison with the ms ticks counter - will confess I scratched my head for some time over this!

diff --git a/modbus.c b/modbus.c
index a8c355e..c965df6 100644
--- a/modbus.c
+++ b/modbus.c
@@ -54,7 +54,7 @@ static const uint32_t baud[]    = { 2400, 4800, 9600, 19200, 38400, 115200 };
 static const uint16_t silence[] = {   16,    8,    4,     2,     2,      2 };
 
 static modbus_stream_t stream;
-static uint16_t rx_timeout = 0, silence_until = 0, silence_timeout;
+static uint32_t rx_timeout = 0, silence_until = 0, silence_timeout;
 static int16_t exception_code = 0;
 static queue_entry_t queue[MODBUS_QUEUE_LENGTH];
 static modbus_settings_t modbus;

Cheers, Jon.

[terjeio]

Thanks, can confirm it is working well with two clients now.

Great, I appreciate the testing you are doing. Perhaps getting closer remove the "experimental" label from the plugin?

... blame the Huanyang implementation for now

Version 1 of the Huanyang protocol is not Modbus compliant in how the messages are made up so I am not surprised. Should the silence delay be made configurable or should we just increase it?

Here is a new version of the code that I believe is closer to how the final version should be:

spindle.zip

I have incorporated your bug fixes in this.

In driver.c initialization will be like this:

#if MODBUS_ENABLE
    modbus_init(serial2Init(115200), NULL);
#endif

#if SPINDLE_HUANYANG > 0
    huanyang_init();
#endif
...

I have added a call for checking if the Modbus plugin managed to install itself:
bool modbus_enabled (void);
this can be called in the initialization function in plugins that wants to be sure that it is available.

Note that the Modbus plugin is not fully configured before settings has been read and acted upon, for that the call
bool modbus_isup (void);
can be used to check the final status.
Both are called from the huanyang code.

Finally the MODBUS_ENABLE symbol has been moved out of modbus.h, you should add it to my_machine.h while testing:
#define MODBUS_ENABLE 1
I will add this in the next commit, but have to update all drivers where the spindle plugin is an option first.

#define MODBUS_ENABLE 2 will also be possible, this will claim the highest numbered auxillary output pin for use as the direction signal.

[dresco]

Great, I appreciate the testing you are doing. Perhaps getting closer remove the "experimental" label from the plugin?
Version 1 of the Huanyang protocol is not Modbus compliant in how the messages are made up so I am not surprised. Should the silence delay be made configurable or should we just increase it?

Thanks, I think I've given it a good testing, but I do have a smaller Huanyang VFD on it's way from China (currently testing with an old 4kW that I had here already). When the new one arrives, I'll compare the silence timeouts and see whether they're consistent. Could then maybe increase the default modbus timeouts to something more suitable if SPINDLE_HUANYANG is defined?

Have noticed that the very first command on startup always ends up in a timeout, will dig into that also...

[dresco]

That initial error was when trying to read the configured speed at 50Hz, was just requesting the wrong register number and getting a NAK. (The default value for rpm_max50 matched the common Chinese spindles, so displayed RPMs were still as expected)..

diff --git a/huanyang.c b/huanyang.c
index 00c1994..7fbb113 100644
--- a/huanyang.c
+++ b/huanyang.c
@@ -331,7 +331,7 @@ static void huanyang_settings_changed (settings_t *settings)
                 .adu[0] = VFD_ADDRESS,
                 .adu[1] = ModBus_ReadCoils,
                 .adu[2] = 0x03,
-                .adu[3] = 122,
+                .adu[3] = 144,
                 .tx_length = 8,
                 .rx_length = 8
             };

That makes me wonder though. This only gets called via huanyang_settings_changed() at startup, so if the spindle isn't responding immediately, then there's no way to get this value without power cycling the board (which is probably tricky once in a cabinet). Perhaps should be called after a reset also?

[terjeio]

Could then maybe increase the default modbus timeouts to something more suitable if SPINDLE_HUANYANG is defined?

Yes, I guess that would be ok.

This only gets called via huanyang_settings_changed() at startup, so if the spindle isn't responding immediately, then there's no way to get this value without power cycling the board (which is probably tricky once in a cabinet). Perhaps should be called after a reset also?

Good idea, I guess you have seen how I do that in the modbus code so you can add it? The original reset handler should be called first so that the modbus code is ready to accept new commands.

[dresco]

Good idea, I guess you have seen how I do that in the modbus code so you can add it? The original reset handler should be called first so that the modbus code is ready to accept new commands.

Yep sure, will do..

[dresco]

This seems to work fine.

One strange thing though - if the VFD is not responding (i.e. powered off for testing), then no spindle alarm gets raised from the modbus timeout when called from the reset path. rx_exception() gets called, but not raise_alarm()?

diff --git a/huanyang.c b/huanyang.c
index 00c1994..2fd8345 100644
--- a/huanyang.c
+++ b/huanyang.c
@@ -63,6 +63,7 @@ static spindle_state_t vfd_state = {0};
 static spindle_data_t spindle_data = {0};
 static settings_changed_ptr settings_changed;
 static on_report_options_ptr on_report_options;
+static driver_reset_ptr driver_reset;
 static uint32_t rpm_max = 0;
 #if SPINDLE_HUANYANG == 1
 static float rpm_max50 = 3000;
@@ -76,6 +77,40 @@ static const modbus_callbacks_t callbacks = {
     .on_rx_exception = rx_exception
 };
 
+// Read maximum configured RPM from spindle, value is used later for calculating current RPM
+// In the case of the original Huanyang protocol, the value is the configured RPM at 50Hz
+static void spindleGetMaxRPM (void)
+{
+#if SPINDLE_HUANYANG == 2
+    modbus_message_t cmd = {
+        .context = (void *)VFD_GetMaxRPM,
+        .adu[0] = VFD_ADDRESS,
+        .adu[1] = ModBus_ReadHoldingRegisters,
+        .adu[2] = 0xB0,
+        .adu[3] = 0x05,
+        .adu[4] = 0x00,
+        .adu[5] = 0x02,
+        .tx_length = 8,
+        .rx_length = 8
+    };
+    modbus_send(&cmd, &callbacks, true);
+#else
+    modbus_message_t cmd = {
+        .context = (void *)VFD_GetMaxRPM50,
+        .adu[0] = VFD_ADDRESS,
+        .adu[1] = ModBus_ReadCoils,
+        .adu[2] = 0x03,
+        .adu[3] = 0x90, // PD144
+        .adu[4] = 0x00,
+        .adu[5] = 0x00,
+        .tx_length = 8,
+        .rx_length = 8
+    };
+    modbus_send(&cmd, &callbacks, true);
+#endif
+}
+
 static void spindleSetRPM (float rpm, bool block)
 {
 
@@ -265,6 +300,12 @@ static void onReportOptions (bool newopt)
     }
 }
 
+static void huanyang_reset (void)
+{
+    driver_reset();
+    spindleGetMaxRPM();
+}
+
 static void huanyang_settings_changed (settings_t *settings)
 {
     static bool init_ok = false;
@@ -305,40 +346,7 @@ static void huanyang_settings_changed (settings_t *settings)
             hal.driver_cap.spindle_at_speed = On;
             hal.driver_cap.spindle_dir = On;
         }
-
-#if SPINDLE_HUANYANG == 2
-        if(!init_ok) {
-
-            modbus_message_t cmd = {
-                .context = (void *)VFD_GetMaxRPM,
-                .adu[0] = VFD_ADDRESS,
-                .adu[1] = ModBus_ReadHoldingRegisters,
-                .adu[2] = 0xB0,
-                .adu[3] = 0x05,
-                .adu[4] = 0x00,
-                .adu[5] = 0x02,
-                .tx_length = 8,
-                .rx_length = 8
-            };
-
-            modbus_send(&cmd, &callbacks, true);
-        }
-#else
-        if(!init_ok) {
-
-            modbus_message_t cmd = {
-                .context = (void *)VFD_GetMaxRPM50,
-                .adu[0] = VFD_ADDRESS,
-                .adu[1] = ModBus_ReadCoils,
-                .adu[2] = 0x03,
-                .adu[3] = 122,
-                .tx_length = 8,
-                .rx_length = 8
-            };
-
-            modbus_send(&cmd, &callbacks, true);
-        }
-#endif
+        spindleGetMaxRPM();
     }
 
     init_ok = true;
@@ -353,6 +361,9 @@ void huanyang_init (void)
         on_report_options = grbl.on_report_options;
         grbl.on_report_options = onReportOptions;
 
+        driver_reset = hal.driver_reset;
+        hal.driver_reset = huanyang_reset;
+
         if(!hal.driver_cap.dual_spindle)
             huanyang_settings_changed(&settings);
     }

[terjeio]

no spindle alarm gets raised from the modbus timeout when called from the reset path. rx_exception() gets called, but not raise_alarm()?

This is due to the rt command queue beeing emptied on a warm reset in protocol.c:

    if(sys.cold_start) {
        hal.spindle.set_state((spindle_state_t){0}, 0.0f);
        hal.coolant.set_state((coolant_state_t){0});
        if(realtime_queue.head != realtime_queue.tail)
            system_set_exec_state_flag(EXEC_RT_COMMAND);  // execute any boot up commands
    } else
        memset(&realtime_queue, 0, sizeof(realtime_queue_t));

During a cold start alarms should only be raised after everything is up and running, that is why I queued it. Since rx_exeception() is called from the foreground I guess it is safe to check sys.cold_start and raise the alarm immediately when false.