hm2_modbus stops sending commands, increments

[JTrantow]

I'm using latest master rip and seeing hm2_modbus lose communications with a CommanderSK VFD via a 7i96 pkuart.

The communications will be fine for several vfd on/off cycles but eventually communications won't turn the VFD on/off.

When this occurs, I see a constantly increasing count (approx 1000 counts per second) on one of the modbus.0.commands.XX.errors counts. It is not stopping at 5. The XX has always been a read command but which one seems random. I started with 11 commands (9 reads, 2 writes).

modbus.0.commands.XX.error_code = 42 (ENOMSG)
modbus.0.commands.XX.disabled = true
modbus.0.commands.XX.reset doesn't seem to have any effect.
modbus.0.commands.XX.disabled doesn't seem to have any effect.

All the other commands have error count =0, but I can't disable or reset the command with a problem.

When this occurs,
modbus.0.last-error-code = 0
modbus.0.fault_command = 0
modbus.0.suspend has no effect.
modbus.0.reset has no effect.

By "has no effect", I see the particular command.XX.error code continue to increment, approx every 1msec, and it doesn't clear or stop incrementing.
Communications are 38.4K baud and there are currently 9 read commands (bunch of register reads to debug what's going on)

Docs indicate suspend is probed at end of list, maybe it is stuck in a 1msec loop incrementing the command error and never makes it to the end of the list???

Docs indicate the .errors count should max out at 5. This is not happening.

Using the 2.9.8, I remember seeing .errors count up to 5 and I could reset them, may have been a different error code. Master version does NOT stop counting at 5 with error_code=42.

[JTrantow]

I examined mesa_modbus.c and changed the MSG_WARN to

` MSG_ERR("%s: warning: reply to command %u had too long inter-character delay (%u > %u), dropping\n",
inst->name, inst->cmdidx,
HM2_PKTUART_RCR_ICHARBITS_VAL(frsize), inst->maxicharbits);

When I ran this change, the message reported inter character delay of 36 when it was automatically set to 29. I increased icdelay="40" and I have not locked up in the last few hours working on the lathe.

The CommanderSK is a fairly old VFD, maybe it just needs a longer icdelay? Even increasing icdelay fixes this case, there is still a bug handling the ENOMSG error. This condition might be repeatable by setting small icdelay values.`

[BsAtHome]

Thanks for the report and digging into the problem.

Just a thought... When the inter-character delay causes problems, then you may be communicating too fast with the device and it responds slower than it should because the characters are sent at the same pace but the bits end up faster on the line. Therefore you end up with longer pauses between the characters. You may also try to reduce the baudrate to see what effect that has.

A error'ed command not disabling automatically after 5 repeated errors is a problem. Especially that it increments the error counter at the servo thread speed.

The problem seems to be that the state machine is waiting for data in state STATE_FETCH_DATA and then the ic-delay error occurs. However, the state machine does not advance and stays in that state stuck forever. Hence it is triggered once every servo period. The problem is that it does not perform a queue_reset(inst) after detecting the error that would make it leave the state.

If you could be so kind, please add a queue_reset(inst) in the if statement block just before the break (at the end of the block where you changed the MSG_WARN to MSG_ERR). That should make it leave the state and resume the state machine after 25 ms. If that works as expected, then I'll make the patch.

[JTrantow]

That all makes sense. I'm away from machine until Monday. I will make change and report back Monday or Tuesday

…

On Sat, Mar 7, 2026, 4:35 AM BsAtHome ***@***.***> wrote: *BsAtHome* left a comment (LinuxCNC/linuxcnc#3849) <#3849 (comment)> Thanks for the report and digging into the problem. Just a thought... When the inter-character delay causes problems, then you may be communicating too fast with the device and it responds slower than it should because the characters are sent at the same pace but the bits end up faster on the line. Therefore you end up with longer pauses between the characters. You may also try to reduce the baudrate to see what effect that has. A error'ed command not disabling automatically after 5 repeated errors is a problem. Especially that it increments the error counter at the servo thread speed. The problem seems to be that the state machine is waiting for data in state STATE_FETCH_DATA and then the ic-delay error occurs. However, the state machine does not advance and stays in that state stuck forever. Hence it is triggered once every servo period. The problem is that it does not perform a queue_reset(inst) after detecting the error that would make it leave the state. If you could be so kind, please add a queue_reset(inst) in the if statement block just before the break (at the end of the block where you changed the MSG_WARN to MSG_ERR). That should make it leave the state and resume the state machine after 25 ms. If that works as expected, then I'll make the patch. — Reply to this email directly, view it on GitHub <#3849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHDVLOQNL6UCFIC2SRE6TT4PP3OPAVCNFSM6AAAAACWJ7RVF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAMJWGIYTGOBRGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[BsAtHome]

Fine, I'll await the results.

There may be another way to deal with the inter-character delay issue. There is a break in that if-clause after the force_resend(inst) statement that prevents the state from changing. My suggestion was to add the queue_reset, forcing the change, but the alternative is simply to remove the break and let the packet read process continue. It will still set the error, but it will try to makes sense of what was received.

It may be preferable not to issue a queue reset because it hard-resets the hardware, which is most likely not required because this is a remote client issue. If things are bad and the read fails, then other errors will pop up and the queue reset should become triggered elsewhere. I'd appreciate it if you could test this scenario.

Yet another bug was discovered when I was reviewing the different code paths to set the inter-packet (not a typo) timing. It is in calc_ifdelay() and uses <= instead of > in the baudrate comparison. That needs to be fix too. Slow baudrates use 3.5 char inter-packet delay, but baudrates > 19200 should adhere to 1750 µs (unless you override and set in explicitly in the mbccs). You are running at 38k4, which makes the difference, assuming 8n1, between 35 (wrong) and 68 (correct). That would mean that the inter-character delay could interfere with the inter-packet delay.

[JTrantow]

I was able to sneak if a quick compile and verify that the code failed (with my latest command set and default interval setting). Then I added the queue reset and it has been running for two hours without any problems. (Did not accumulate 5 errors to disable any commands.)

#if (0)
// Fix 1. This works.
queue_reset(inst);
break;
#else
// Fix 2.
// Test without resetting the queue and without the break.
#endif

As for the baudrate, I am using and testing with 38.4 N2 and seeing 29 in the parameter. I will retest with Fix 2 and leave the baud fix alone as it's more likely to trigger the condition this way.

When I looked at set_error(), one concern I have is that 5 errors disables the command and I don't seen any warning/error mechanism. I think there should definitely be a dialog if a command gets disabled and the other commands continue to run.

[JTrantow]

I let Fix2 run overnight without any problems. I can submit a pull request with just this fix or I could implement and test with the baud rate fix???

[JTrantow]

Fix2 (removing break) ran overnight without any problems.

I implemented the baud rate fix and observed the 38400 8N2 rxdelay go from 38 -> 67, txdelay 40 -> 69. I will let this run for several hours and submit a pull request.

[BsAtHome]

Fix 2 is the right one. Actually, the force_resend() also needs to go because if the transmission succeeded, then you want to honor the interval timing.

The inter-packet delay calculation on the baudrate should be tested as well, but I'm fairly sure that it is probably cosmetic when fix2 already functions as expected.

BTW, if you run on an updated master (2.10-pre), then you should be able to see the printout of the config at startup. Something like:

hm2_modbus.0: inst->icdelay  : ...
hm2_modbus.0: inst->rxdelay  : ...
hm2_modbus.0: inst->txdelay  : ...

where the ... are some values. If rxdelay or txdelay are less than icdelay, then that is a dead giveaway that the auto calculation was off.

As far as I can tell, this is the diff required:

diff --git a/src/hal/drivers/mesa-hostmot2/hm2_modbus.c b/src/hal/drivers/mesa-hostmot2/hm2_modbus.c
index d186ebeb14..56d091cef0 100644
--- a/src/hal/drivers/mesa-hostmot2/hm2_modbus.c
+++ b/src/hal/drivers/mesa-hostmot2/hm2_modbus.c
@@ -387,7 +387,7 @@ static unsigned calc_ifdelay(hm2_modbus_inst_t *inst, unsigned baudrate, unsigne
 	}
 
 	// calculation works for baudrates less than ~24 Mbit/s
-	if(baudrate <= 19200)
+	if(baudrate > 19200)
 		return (175u * baudrate + 99999u) / 100000u;
 	unsigned bits = 1 + 8 + (parity ? 1 : 0) + (stopbits > 1 ? 2 : 1);
 	return (bits * 35 + 9) / 10;	// Bit-times * 3.5 rounded up
@@ -1049,12 +1049,10 @@ fetch_more_data:
 			break;
 		}
 		if(inst->maxicharbits && HM2_PKTUART_RCR_ICHARBITS_VAL(frsize) > inst->maxicharbits) {
-			MSG_WARN("%s: warning: reply to command %u had too long inter-character delay (%u > %u), dropping\n",
+			MSG_WARN("%s: warning: reply to command %u had too long inter-character delay (%u > %u), trying to interpret\n",
 					inst->name, inst->cmdidx,
 					HM2_PKTUART_RCR_ICHARBITS_VAL(frsize), inst->maxicharbits);
 			set_error(inst, ENOMSG);
-			force_resend(inst);
-			break;
 		}
 		inst->rxdata[0] = 0;	// This will fail the parse packet if the read did not resolve
 		r = hm2_pktuart_queue_read_data(inst->uart, inst->rxdata, HM2_PKTUART_RCR_NBYTES_VAL(frsize));

Please note that any PR needs to be backported as well.

[BsAtHome]

When I looked at set_error(), one concern I have is that 5 errors disables the command and I don't seen any warning/error mechanism. I think there should definitely be a dialog if a command gets disabled and the other commands continue to run.

That is not possible. This is code that runs in the realtime realm and you cannot make dialogs there or delay anything, ever.

Warnings and errors should be emitted in the GUI. The driver uses the rtapi message API. But you need the appropriate message level for errors (level=1). Errors and warnings are only visible when the message level is set to display both (level=2). If you have the proper message level, then you should have seen 5 messages before a command gets disabled.

Writing an extra message was not deemed to be necessary, but pro/con can be discussed. Intermittent errors don't disable the command. The error counter resets when the command succeeds in STATE_HANDLE_DATA when the parsing the data frame of the command succeeds.

[JTrantow]

Ok, I just started a quick test after removing the force_resend(). I'll let that run for an hour or two and submit a PR. My changes are identical to your diff, except I added comment for the return being in units of bits and corrected round comment to ceil.

diff.txt

[BsAtHome]

Well, that comment comments ;-)

For understanding the meaning of all these values you need to read the PktUART interface docs and the "Modbus over serial line 1.02" doc.

The change to "- 1750 microseconds worth of bits when baudrate > 19200 and <= 582000" is not correct. You comment on the implementation whereas I commented on the Modbus specification. The implementation has a limit because the hardware has a limitation. That is why the code does what it does and limits at 582kBaud with a warning (!). That does not mean that the inter-frame delay should be a different value than 1750 µs for higher baurates. Therefore, IMO, the original statement should prevail.

Now I'm nitpicking... calling it ceil is in my world only correct for floating point. But well, my brain is bits and bytes too.

[JTrantow]

(Please read this picturing me with a grin.)

In my pre-retirement world, functions always specified the units and values they returned to make it easier for writing functional tests. :) And yes, comments in code described the implementation of the function. But, I also never wrote functions with multiple return statements, or used conditional operators that weren't wrapped within a macro, or didn't check input argument validity, or used "magic numbers" like 1020/1750 in code . I wrote quite a bit of MISRA compliant code over the years and all these practices were not allowed.

As for ceil(), I default to mathmatical analysis:

For all x members of Real Numbers, ceil(x) is defined as the minimum n member of integers such that n>=x.

Neither the specification, nor implementation of ceil in the case of calculating number of timing bits has anything to do with floating point.

In integer math, round(A/B) is often implemented as (A+B/2))/B, ceil as (A+(B-1))/B round and ceil are not the same.

Don't get me started on C99 fixed width or Qm.n fixed point implementations...

My preference is to write something along the lines of:
`/*!
\brief Modbus parity.
*/
typedef enum parity {
NO_PARITY = 0, //!< No parity.
EVEN_PARITY, //!< Even parity.
ODD_PARITY //!< Odd parity.
} parity_type;

/*!
\brief Modbus stop bits.
*/
typedef enum stop_bits {
ONE_STOP = 1, //!< One stop bit.
TWO_STOP = 2 //!< Two stop bits.
} stop_bits_type;

/*!
\brief Calculate the inter-frame delay time in units of numbers of bits. (\todo is this hardware limited to uint16_t, does it really need to be rtapi_u32?)

Notice ASCII protocol only has seven data bits, this must be RTU?

\param inst Pointer to modbus instance.
\param baudrate Modbus baud setting.
\param parity Modbus parity setting.
\param stopbits Modbus stopbits setting.

\retvalue 3.5 chars worth of bits if baudrate <= 19200
\retvalue 1750 microseconds worth of bits when baudrate > 19200 and <= 582000.
\retvalue HARDWARE_MAX_DELAY When baud > 582000

*/
static rtapi_u32 calc_ifdelay2(const hm2_modbus_inst_t * const inst, unsigned baudrate, parity_type parity, stop_bits_type stopbits)
{
unsigned timing_bits;

if (19200 >= baudrate ) {
	/*
		Slow bauds require at least 3.5 characters of bits.
	*/
	unsigned bits_per_char;
	if (NO_PARITY == parity) {
		bits_per_char = 1 + 8 + stopbits;		// One start, eight data, and stop bits.
	}else {
		bits_per_char = 1 + 8 + 1 + stopbits;  // One start, eight data, one parity and stop bits.
	}
	timing_bits = ((bits_per_char * 35 + 9) / 10);	// Ceil of bits in 3.5 characters.
}else if (HARDWARE_BAUD_LIMIT >= baudrate ) {
	/*
		Above 19.2K requires 1750 microseconds worth of bits.
	*/
	timing_bits = (175u * baudrate + 99999u) / 100000u;	// calculation works for baudrates less than ~24 Mbit/s
}else{
	/*
		Hardware is limited to HARDWARE_MAX_DELAY bits.
	*/
	MSG_WARN("%s: warning: Baudrate > 582000 will make inter-frame timer overflow. Setting to maximum.\n", inst->name);
	timing_bits = HARDWARE_MAX_DELAY;
}
return(timing_bits);

}
`

[JTrantow]

Thanks for your help. My lathe has been running for hours without any problems.
I submitted a pull request to the master.
I still don't understand exactly how LCNC is managed. What do you mean by backport? Same PR to 2.9 branch?

Feel free to change my code or comments as you see fit. It's really your code, I just tested. Thanks again.

[BsAtHome]

Thanks for the PR.

Yes, it needs backporting to 2.9 branch. Usually, it is done in reverse, fix 2.9 and that gets forward ported. However, this code will very often conflict in whatever direction you try. The code bases diverged too far between branches for automatic handling. But when you look at the code bases, then you'll see that these core functions all are still the same, just usually offset quite bit.