Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alexrudd2 wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2d40de
Return 0x02 Illegal Data Address instead of 0x04 Server Device Failure
alexrudd2 Aug 6, 2025
777fae9
Use a custom Exception instead of broad KeyError
alexrudd2 Aug 8, 2025
d199c5e
Merge branch 'dev' into issue-2716
janiversen Aug 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pymodbus/datastore/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
from collections.abc import Iterable
from typing import Any, Generic, TypeVar

from pymodbus.exceptions import ParameterException
from pymodbus.exceptions import NoSuchAddressException, ParameterException


# ---------------------------------------------------------------------------#
Expand Down Expand Up @@ -259,8 +259,12 @@ def getValues(self, address, count=1):
:param address: The starting address
:param count: The number of values to retrieve
:returns: The requested values from a:a+c
:raises: NoSuchAddressException
"""
return [self.values[i] for i in range(address, address + count)]
try:
return [self.values[i] for i in range(address, address + count)]
except KeyError as e:
raise NoSuchAddressException(str(e)) from e
Copy link
Collaborator

@janiversen janiversen Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@alexrudd2 alexrudd2 Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@janiversen janiversen Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@alexrudd2 alexrudd2 Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


def _process_values(self, values):
"""Process values."""
Expand Down
8 changes: 8 additions & 0 deletions pymodbus/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ def __init__(self, string=""):
message = f"[No Such id] {string}"
ModbusException.__init__(self, message)

class NoSuchAddressException(ModbusException):
Copy link
Collaborator

@janiversen janiversen Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? 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.

Copy link
Collaborator Author

@alexrudd2 alexrudd2 Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Error resulting from making a request for a register that does not exist."""

def __init__(self, string=""):
"""Initialize."""
message = f"[No such register address] {string}"
ModbusException.__init__(self, message)


class NotImplementedException(ModbusException):
"""Error resulting from not implemented function."""
Expand Down
9 changes: 8 additions & 1 deletion pymodbus/server/requesthandler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import asyncio
import traceback

from pymodbus.exceptions import ModbusIOException, NoSuchIdException
from pymodbus.exceptions import (
ModbusIOException,
NoSuchAddressException,
NoSuchIdException,
)
from pymodbus.logging import Log
from pymodbus.pdu.pdu import ExceptionResponse
from pymodbus.transaction import TransactionManager
Expand Down Expand Up @@ -105,6 +109,9 @@ async def handle_request(self):
if self.server.ignore_missing_devices:
return # the client will simply timeout waiting for a response
response = ExceptionResponse(self.last_pdu.function_code, ExceptionResponse.GATEWAY_NO_RESPONSE)
except NoSuchAddressException:
Copy link
Collaborator

@janiversen janiversen Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Log.error("Requested register address does not exist: {}", self.last_pdu.address)
response = ExceptionResponse(self.last_pdu.function_code, ExceptionResponse.ILLEGAL_ADDRESS)
except Exception as exc: # pylint: disable=broad-except
Log.error(
"Datastore unable to fulfill request: {}; {}",
Expand Down