Skip to content
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.

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

async datastore in modbus server #2127

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions examples/server_callback.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ def __init__(self, queue, addr, values):
self.queue = queue
super().__init__(addr, values)

def setValues(self, address, value):
async def async_setValues(self, address, value):
"""Set the requested values of the datastore."""
super().setValues(address, value)
await super().async_setValues(address, value)
txt = f"Callback from setValues with address {address}, value {value}"
_logger.debug(txt)

def getValues(self, address, count=1):
async def async_getValues(self, address, count=1):
"""Return the requested values from the datastore."""
result = super().getValues(address, count=count)
result = await super().async_getValues(address, count=count)
txt = f"Callback from getValues with address {address}, count {count}, data {result}"
_logger.debug(txt)
return result
Expand All @@ -56,7 +56,7 @@ async def run_callback_server(cmdline=None):
"""Define datastore callback for server and do setup."""
queue = asyncio.Queue()
block = CallbackDataBlock(queue, 0x00, [17] * 100)
block.setValues(1, 15)
await block.async_setValues(1, 15)
store = ModbusSlaveContext(di=block, co=block, hr=block, ir=block)
context = ModbusServerContext(slaves=store, single=True)
run_args = server_async.setup_server(
Expand Down
10 changes: 6 additions & 4 deletions examples/server_updating.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ async def updating_task(context):
count = 6

# set values to zero
values = context[slave_id].getValues(fc_as_hex, address, count=count)
values = await context[slave_id].async_getValues(fc_as_hex, address, count=count)
values = [0 for v in values]
context[slave_id].setValues(fc_as_hex, address, values)
await context[slave_id].async_setValues(fc_as_hex, address, values)

txt = (
f"updating_task: started: initialised values: {values!s} at address {address!s}"
Expand All @@ -75,9 +75,11 @@ async def updating_task(context):
while True:
await asyncio.sleep(2)

values = context[slave_id].getValues(fc_as_hex, address, count=count)
values = await context[slave_id].async_getValues(
fc_as_hex, address, count=count
)
values = [v + 1 for v in values]
context[slave_id].setValues(fc_as_hex, address, values)
await context[slave_id].async_setValues(fc_as_hex, address, values)

txt = f"updating_task: incremented values: {values!s} at address {address!s}"
print(txt)
Expand Down
12 changes: 8 additions & 4 deletions pymodbus/bit_read_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def __init__(self, address=None, count=None, slave=0, **kwargs):
"""
ReadBitsRequestBase.__init__(self, address, count, slave, **kwargs)

def execute(self, context):
async def execute(self, context):
ilkka-ollakka marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also called from the SYNC client.

"""Run a read coils request against a datastore.

Before running the request, we make sure that the request is in
Expand All @@ -169,7 +169,9 @@ def execute(self, context):
return self.doException(merror.IllegalValue)
if not context.validate(self.function_code, self.address, self.count):
return self.doException(merror.IllegalAddress)
values = context.getValues(self.function_code, self.address, self.count)
values = await context.async_getValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This too is not exclusive for the server.

self.function_code, self.address, self.count
)
if isinstance(values, ExceptionResponse):
return values
return ReadCoilsResponse(values)
Expand Down Expand Up @@ -223,7 +225,7 @@ def __init__(self, address=None, count=None, slave=0, **kwargs):
"""
ReadBitsRequestBase.__init__(self, address, count, slave, **kwargs)

def execute(self, context):
async def execute(self, context):
"""Run a read discrete input request against a datastore.

Before running the request, we make sure that the request is in
Expand All @@ -237,7 +239,9 @@ def execute(self, context):
return self.doException(merror.IllegalValue)
if not context.validate(self.function_code, self.address, self.count):
return self.doException(merror.IllegalAddress)
values = context.getValues(self.function_code, self.address, self.count)
values = await context.async_getValues(
self.function_code, self.address, self.count
)
if isinstance(values, ExceptionResponse):
return values
return ReadDiscreteInputsResponse(values)
Expand Down
14 changes: 9 additions & 5 deletions pymodbus/bit_write_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def decode(self, data):
self.address, value = struct.unpack(">HH", data)
self.value = value == ModbusStatus.ON

def execute(self, context):
async def execute(self, context):
"""Run a write coil request against a datastore.

:param context: The datastore to request from
Expand All @@ -90,10 +90,12 @@ def execute(self, context):
if not context.validate(self.function_code, self.address, 1):
return self.doException(merror.IllegalAddress)

result = context.setValues(self.function_code, self.address, [self.value])
result = await context.async_setValues(
self.function_code, self.address, [self.value]
)
if isinstance(result, ExceptionResponse):
return result
values = context.getValues(self.function_code, self.address, 1)
values = await context.async_getValues(self.function_code, self.address, 1)
if isinstance(values, ExceptionResponse):
return values
return WriteSingleCoilResponse(self.address, values[0])
Expand Down Expand Up @@ -212,7 +214,7 @@ def decode(self, data):
values = unpack_bitstring(data[5:])
self.values = values[:count]

def execute(self, context):
async def execute(self, context):
"""Run a write coils request against a datastore.

:param context: The datastore to request from
Expand All @@ -226,7 +228,9 @@ def execute(self, context):
if not context.validate(self.function_code, self.address, count):
return self.doException(merror.IllegalAddress)

result = context.setValues(self.function_code, self.address, self.values)
result = await context.async_setValues(
self.function_code, self.address, self.values
)
if isinstance(result, ExceptionResponse):
return result
return WriteMultipleCoilsResponse(self.address, count)
Expand Down
46 changes: 43 additions & 3 deletions pymodbus/datastore/context.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
"""Context for datastore."""

# pylint: disable=missing-type-doc
from pymodbus.datastore.store import ModbusSequentialDataBlock
from pymodbus.exceptions import NoSuchSlaveException
from pymodbus.logging import Log


class ModbusBaseSlaveContext: # pylint: disable=too-few-public-methods
class ModbusBaseSlaveContext:
"""Interface for a modbus slave data context.

Derived classes must implemented the following methods:
reset(self)
validate(self, fx, address, count=1)
getValues(self, fx, address, count=1)
setValues(self, fx, address, values)
getValues(self, fc_as_hex, address, count=1) or async_getValues(self, fc_as_hex, address, count=1)
setValues(self, fc_as_hex, address, values) or async_setValues(self, fc_as_hex, address, values)
"""

_fx_mapper = {2: "d", 4: "i"}
Expand All @@ -27,6 +28,45 @@ def decode(self, fx):
"""
return self._fx_mapper[fx]

async def async_getValues(self, fc_as_hex, address, count=1):
"""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)

async def async_setValues(self, fc_as_hex, address, values):
"""Set the datastore with the supplied values.

:param fc_as_hex: The function we are working with
:param address: The starting address
:param values: The new values to be set
"""
return self.setValues(fc_as_hex, address, values)

def getValues(self, fc_as_hex, address, count=1):
"""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
"""
del fc_as_hex, address, count
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why delete the variables, that does not make a lot of sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are not used, so makes it more clear in that point. Mypy and ruff etc easily complain on those. But I agree it is more of taste and not really critical to be in my point of view.

Copy link
Collaborator

Choose a reason for hiding this comment

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

use "_" that is the standard way to tell a variable is unused.

It might not be critical from your POW, but from me as maintainer it is negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I can change it to be with "_".

return []

def setValues(self, fc_as_hex, address, values):
"""Set the datastore with the supplied values.

:param fc_as_hex: The function we are working with
:param address: The starting address
:param values: The new values to be set
"""
del fc_as_hex, address, values
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.



# ---------------------------------------------------------------------------#
# Slave Contexts
Expand Down
73 changes: 31 additions & 42 deletions pymodbus/datastore/remote.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Remote datastore."""

from pymodbus.datastore import ModbusBaseSlaveContext
from pymodbus.exceptions import NotImplementedException
from pymodbus.logging import Log
Expand Down Expand Up @@ -38,25 +39,37 @@ def validate(self, _fc_as_hex, _address, _count):
"""
return True

def getValues(self, fc_as_hex, _address, _count=1):
async def async_getValues(self, fc_as_hex, address, count=1):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left these changes as separate commit, incase there is some desire to keep this still intact.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is the other way around, we do not want the cascade changes your original PR contained and still contains.

Having the async_getValues, should solve your problem, so please revert all the cascaded changes in the system (your PR still touches 31 files).

"""Get values from real call in validate."""
if fc_as_hex in self._write_fc:
return [0]
group_fx = self.decode(fc_as_hex)
func_fc = self.__get_callbacks[group_fx]
self.result = func_fc(_address, _count)
kwargs = {}
if self.slave:
kwargs["slave"] = self.slave
self.result = await getattr(self._client, func_fc)(address, count, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks wrong to me, the code is designed so we do not need to call getattr, which is very expensive function.

return self.__extract_result(self.decode(fc_as_hex), self.result)

def setValues(self, fc_as_hex, address, values):
async def async_setValues(self, fc_as_hex, address, values):
"""Set the datastore with the supplied values."""
group_fx = self.decode(fc_as_hex)
if fc_as_hex not in self._write_fc:
raise ValueError(f"setValues() called with an non-write function code {fc_as_hex}")
raise ValueError(
f"setValues() called with an non-write function code {fc_as_hex}"
)
func_fc = self.__set_callbacks[f"{group_fx}{fc_as_hex}"]
kwargs = {}
if self.slave:
kwargs["slave"] = self.slave
if fc_as_hex in {0x0F, 0x10}: # Write Multiple Coils, Write Multiple Registers
self.result = func_fc(address, values)
self.result = await getattr(self._client, func_fc)(
address, values, **kwargs
)
else:
self.result = func_fc(address, values[0])
self.result = await getattr(self._client, func_fc)(
address, values[0], **kwargs
)
if self.result.isError():
return self.result
return None
Expand All @@ -74,44 +87,20 @@ def __build_mapping(self):
if self.slave:
kwargs["slave"] = self.slave
self.__get_callbacks = {
"d": lambda a, c: self._client.read_discrete_inputs(
a, c, **kwargs
),
"c": lambda a, c: self._client.read_coils(
a, c, **kwargs
),
"h": lambda a, c: self._client.read_holding_registers(
a, c, **kwargs
),
"i": lambda a, c: self._client.read_input_registers(
a, c, **kwargs
),
"d": "read_discrete_inputs",
"c": "read_coils",
"h": "read_holding_registers",
"i": "read_input_registers",
}
self.__set_callbacks = {
"d5": lambda a, v: self._client.write_coil(
a, v, **kwargs
),
"d15": lambda a, v: self._client.write_coils(
a, v, **kwargs
),
"c5": lambda a, v: self._client.write_coil(
a, v, **kwargs
),
"c15": lambda a, v: self._client.write_coils(
a, v, **kwargs
),
"h6": lambda a, v: self._client.write_register(
a, v, **kwargs
),
"h16": lambda a, v: self._client.write_registers(
a, v, **kwargs
),
"i6": lambda a, v: self._client.write_register(
a, v, **kwargs
),
"i16": lambda a, v: self._client.write_registers(
a, v, **kwargs
),
"d5": "write_coil",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the same...but might be the reason I saw a getattr earlier.

We do not want that, apart from that what does it have to do with getValues being async ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is mainly due lamdas and await don't work together. It can be redone without getattr use most likely easily.

"d15": "write_coils",
"c5": "write_coil",
"c15": "write_coils",
"h6": "write_register",
"h16": "write_registers",
"i6": "write_register",
"i16": "write_registers",
}
self._write_fc = (0x05, 0x06, 0x0F, 0x10)

Expand Down
14 changes: 14 additions & 0 deletions pymodbus/datastore/simulator.py
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,13 @@ def validate(self, func_code, address, count=1):
fx_write = func_code in self._write_func_code
return self.loop_validate(real_address, real_address + count, fx_write)

async def async_getValues(self, func_code, address, count=1):
"""Return the requested values of the datastore.

:meta private:
"""
return self.getValues(func_code, address, count)

def getValues(self, func_code, address, count=1):
"""Return the requested values of the datastore.

Expand Down Expand Up @@ -611,6 +618,13 @@ def getValues(self, func_code, address, count=1):
bit_index = 0
return result

async def async_setValues(self, func_code, address, values):
"""Set the requested values of the datastore.

:meta private:
"""
return self.setValues(func_code, address, values)

def setValues(self, func_code, address, values):
"""Set the requested values of the datastore.

Expand Down
22 changes: 22 additions & 0 deletions pymodbus/datastore/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ class BaseModbusDataBlock(ABC, Generic[V]):
getValues(self, address, count=1)
setValues(self, address, values)
reset(self)

Derived classes can implemented the following async methods:
async_getValues(self, address, count=1)
async_setValues(self, address, values)
"""

values: V
Expand All @@ -87,6 +91,15 @@ def validate(self, address:int, count=1) -> bool:
:raises TypeError:
"""

async def async_getValues(self, address: int, count=1) -> Iterable:
"""Return the requested values from the datastore.

:param address: The starting address
:param count: The number of values to retrieve
:raises TypeError:
"""
return self.getValues(address, count)

@abstractmethod
def getValues(self, address:int, count=1) -> Iterable:
"""Return the requested values from the datastore.
Expand All @@ -96,6 +109,15 @@ def getValues(self, address:int, count=1) -> Iterable:
:raises TypeError:
"""

async def async_setValues(self, address: int, values):
"""Return the requested values from the datastore.

:param address: The starting address
:param values: The values to store
:raises TypeError:
"""
return self.setValues(address, values)

@abstractmethod
def setValues(self, address:int, values) -> None:
"""Return the requested values from the datastore.
Expand Down
Loading