Return 0x02 Illegal Data Address instead of 0x04 Server Device Failure

[alexrudd2]

Closes #2716

alex@antorak:~/Desktop$ python3 client_test.py 1

--- Reading register 1 ---
Register 1 value: 17

# before change
alex@antorak:~/Desktop$ python3 client_test.py 99

--- Reading register 99 ---
Exception code 0x04
DEVICE FAILURE

# after change
alex@antorak:~/Desktop$ python3 client_test.py 99

--- Reading register 99 

Exception code 0x02
ILLEGAL DATA ADDRESS

[janiversen]

This is not generic, problems in the sparse datastore, should be solved at that level.

keyError can probably mean different things in the different datastores.

Look at simulator.py around line 587, to see how it is handled there (apart from the ugly constant 2).

[janiversen]

Unless I am very mistaken, you make a lot of changes for something that is already implemented in the general datastore and used in the simulator datastore

However I might be wrong....but important is that datastore should handle all datastore problems and not pass the handling to the upper layer.

[janiversen]

?? That it something normal, that the library should handle gracefully (and it does in the simulator datastore).

Exception like e.g. NotImplementedException are there to signal the developers forgot to implement something.

[alexrudd2]

How else, besides bubbling an Exception up, is the response handler supposed to know what the underlying error was?

[janiversen]

Why raise an exception, that is not the pattern we normally use....exceptions are raised when the library encounters a problem that was not covered by code.

You can just return ExceptionResponse.ILLEGAL_ADDRESS, that should work.

[alexrudd2]

getValues (in the happy case) returns a list of values, not a full ModbusResponse.

[janiversen]

Look in simulator.py line 588, it returns a list of values or an integer.

[alexrudd2]

!! Returning a magic value is strange, undocumented, and causing confusing types.

Arguably self.validate() itself should throw the Exception.

[janiversen]

The request handler should really not know about the datastore...it handles requests and returns a response.

[alexrudd2]

Unless I am very mistaken, you make a lot of changes for something that is already implemented in the general datastore and used in the simulator datastore

However I might be wrong....but important is that datastore should handle all datastore problems and not pass the handling to the upper layer.

At least for the sparse datastore, there is an uncaught KeyError in get_values() that bubbles all the way up to the request handler before being caught by except Exception.

pymodbus/pymodbus/server/requesthandler.py

Lines 108 to 114 in b9731ec

	except Exception as exc: # pylint: disable=broad-except
	Log.error(
	"Datastore unable to fulfill request: {}; {}",
	exc,
	traceback.format_exc(),
	)
	response = ExceptionResponse(self.last_pdu.function_code, ExceptionResponse.DEVICE_FAILURE)

This exception handler is overly generic. (Indeed, even pylint complains).
In this case, it can only send ExceptionResponse.DEVICE_FAILURE even if we "know" it should be ExceptionResponse.ILLEGAL_ADDRESS.

[janiversen]

First of all sparse data block is an old relic, that is going to be removed as soon as possible...but anyhow the key error should be caught in the datastore not passed along.

The overly generic exception handler is quite correct, that was made like this to ensure that the server return a valid frame when it encounters a programming problem....you are trying to add a perfectly valid scenario (illegalAddress) to that. Datastore problems should be handled in the datastore module.

[alexrudd2]

.you are trying to add a perfectly valid scenario (illegalAddress) to that.

The entire purpose of this PR is to close #2716.

Any solution which replies with 0x04 Device Failure instead of 0x02 Illegal Address does not accomplish that.
EDIT: Reversed the order

[alexrudd2]

First of all sparse data block is an old relic, that is going to be removed as soon as possible

Noted, I'm expecting to have to refactor my downstream code again for 4.0

[janiversen]

.you are trying to add a perfectly valid scenario (illegalAddress) to that.

The entire purpose of this PR is to close #2716.

Any solution which replies with 0x04 Device Failure instead of 0x02 Illegal Address does not accomplish that. EDIT: Reversed the order

The solution is to do as the datastore simulator does, I even gave you the line number.

[alexrudd2]

The solution is to do as the datastore simulator does, I even gave you the line number.

Jan, I very much appreciate your suggestions for better code, and I'm not trying to argue for its own sake. I urge you to reconsider.

pymodbus/pymodbus/datastore/context.py

Lines 32 to 40 in 9e22049

	async def async_getValues(self, fc_as_hex: int, address: int, count: int = 1) -> list[int] | list[bool] | int:
	"""Get `count` values from datastore.
	:param fc_as_hex: The function we are working with
	:param address: The starting address
	:param count: The number of values to retrieve
	:returns: The requested values from a:a+c
	"""
	return self.getValues(fc_as_hex, address, count)

pymodbus/pymodbus/datastore/simulator.py

Lines 582 to 589 in 9e22049

	def getValues(self, func_code, address, count=1):
	"""Return the requested values of the datastore.
	:meta private:
	"""
	if not self.validate(func_code, address, count):
	return 2
	result = []

With all due respect, this code is bad.

• Returning magic numbers for an error condition is not typical Python at all; it is more common to use Exceptions.

• It's not documented in either get_values() or async_getValues(). Even if it were, what would the docstring be? """"Return the requested values of the datastore, or '2' if the values cannot be found in the datastore.""" 🤮

pyright complains:

mypy would complain as well, if the function were typed.

• The code doesn't even work! The simulator generates inscrutable error message when accessing an invalid register.

2025-08-08 14:39:18,050 ERROR requesthandler:108 Datastore unable to fulfill request: object of type 'int' has no len(); Traceback (most recent call last):
  File "/home/alex/git/pymodbus/pymodbus/server/requesthandler.py", line 100, in handle_request
    response = await self.last_pdu.update_datastore(context)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/alex/git/pymodbus/pymodbus/pdu/register_message.py", line 47, in update_datastore
    return response_class(
           ^^^^^^^^^^^^^^^
  File "/home/alex/git/pymodbus/pymodbus/pdu/pdu.py", line 37, in __init__
    self.count: int = count or len(self.registers)
                               ^^^^^^^^^^^^^^^^^^^
TypeError: object of type 'int' has no len()

• The poor client is left with 0x04 DEVICE_FAILURE. I refer to pages 47-48 available of V1_1b3. 0x04 is "An unrecoverable error occurred while the server was attempting to perform the requested action." That's not true - the error is recoverable.
  The spec is clear that 0x02 should be sent in this case: "If the output address is non–existent in the server device, the server will return the exception response with the exception code shown (02). This specifies an illegal data address for the server."

[janiversen]

You are, as usual right, in a lot of your rant.....BUT at the end of the day there are a couple of design goals which are final:

• A module handles it's own problems (this is mandatory)
• Exceptions are only raised in situations where the developer have made something wrong (this can be a bit flexible but within the module).

Simulator.py do return IllegalAddress, but might break afterwards (at least one user is satisfied with the current solution)....however the datastore are moving in the direction of returning a list is ok and an integer if not ok == modbus exception.

Sequential and sparse data block are being changed to convert internally to the simulator module (which is not the simulator datastore). It means in the future it is not recommended to inherit the datastore and make your own version, but instead define a dict to run with the simulator.

FYI, in the future we will have a server with a relative fixed setup and thus easier to use, and a simulator with a lot more flexibility (including a http interface if activated).

[alexrudd2]

Simulator.py do return IllegalAddress,

No, it does not, or at least not to modbus requests. See my test code - it returns DeviceFailure. Perhaps you're thinking of the HTTP / webpage frontend?

Anyways, to summarize the options I see:
(1) datastores sometimes return different values, request handler fails ==> buggy and needs to be improved
(2) datastores throw exceptions, request handler catches them (BAFP) ==> my recommendation, but rejected for design reasons
(3) request handler checks for any possible datastore errors (LBYL) ==> should work, most maintains existing call signatures
(4) datastore return additional status (error/ok), request handler parses ==> not my favorite, but could also work

[janiversen]

The new datastore goes in direction of 1), but of course you are welcome to suggest other routes, as long as datastore handling stays within the datastore module.

[alexrudd2]

Closing in favor of #2733 (which went with option 4)