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.

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

modbus_server: execute to handle message execute to be coroutine or method #2139

Merged
merged 1 commit into from
Apr 2, 2024
Merged

modbus_server: execute to handle message execute to be coroutine or method #2139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions pymodbus/server/async_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def __init__(self, owner):
self.receive_queue: asyncio.Queue = asyncio.Queue()
self.handler_task = None # coroutine to be run on asyncio loop
self.framer: ModbusFramer
self.loop = asyncio.get_running_loop()

def _log_exception(self):
"""Show log exception."""
Expand Down Expand Up @@ -173,6 +174,9 @@ def execute(self, request, *addr):
if self.server.request_tracer:
self.server.request_tracer(request, *addr)

asyncio.run_coroutine_threadsafe(self._async_execute(request, *addr), self.loop)

async def _async_execute(self, request, *addr):
broadcast = False
try:
if self.server.broadcast_enable and not request.slave_id:
Expand All @@ -181,9 +185,17 @@ def execute(self, request, *addr):
# note response will be ignored
for slave_id in self.server.context.slaves():
response = request.execute(self.server.context[slave_id])
# Temporary check while we move execute to async method
if asyncio.iscoroutine(response):
Copy link
Collaborator

Choose a reason for hiding this comment

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

A short comment telling this is temporary would be nice.

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'll add mention about temporary

response = await response
else:
context = self.server.context[request.slave_id]
response = request.execute(context)

# Temporary check while we move execute to async method
if asyncio.iscoroutine(response):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you move the "if" outside the "else:" you do not need to duplicate the test.

Remark: the "for slave_id" is not the best programming, because it can only be there once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point, I'll move it outside of else block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only problem with it being outside, is that with broadcast it doesn't raise exception in other than last request. But as this being temporary code, I think it is acceptable transition phase?

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 in place, mainly so that it still raises exceptions from execute in broadcast when it happens and it would end up leaving other execute calls to be not awaited.

response = await response

except NoSuchSlaveException:
Log.error("requested slave does not exist: {}", request.slave_id)
if self.server.ignore_missing_slaves:
Expand Down