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

Add async version of send_message for both tcp and rtu #110

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tiagocoutinho
Copy link
Contributor

@tiagocoutinho tiagocoutinho commented Nov 7, 2020

Now that #86 is out I feel more comfortable making this PR since the new async/await python 3 syntax makes it impossible to use with python 2.

This PR proposes to add an async version of send_message for both tcp and rtu.
We will be able to write asyncio based clients like this:

import asyncio

from umodbus import conf
from umodbus.client import tcp

async def main():
    reader, writer = await asyncio.open_connection('localhost', 502)
    message = tcp.write_multiple_coils(slave_id=1, starting_address=1, values=[1, 0, 1, 1])
    response = await tcp.async_send_message(message, reader, writer)
    writer.close()
    await writer.wait_closed()


asyncio.run(main())

We are not restricted to TCP. Any I/O library supporting asyncio StreamReader and StreamWriter interface (example: serial_asyncio) can be used with either the TCP or RTU versions of async_send_message.

@OrangeTux
Copy link
Collaborator

Thanks for this PR! I might need a bit of time to review the PR and test it myself. I'll come back to you.

@tiagocoutinho
Copy link
Contributor Author

You're welcome.
No worries, take your time.

Copy link
Member

@jaapz jaapz left a comment

Choose a reason for hiding this comment

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

Hey @tiagocoutinho , thanks for your contribution. This is a great addition to uModbus. Just had some comments for you to look at, before we can merge.

I'm personally also not a big fan of the async_send_message names. Maybe the async method could be moved to an asyncio module so it could be used like

from umodbus.client.tcp.asyncio import send_message
await send_message(...)

README.rst Outdated Show resolved Hide resolved
scripts/examples/simple_async_tcp_client.py Outdated Show resolved Hide resolved
tests/system/responses/test_exception_async_responses.py Outdated Show resolved Hide resolved
tests/system/responses/test_exception_async_responses.py Outdated Show resolved Hide resolved
tests/system/responses/test_exception_async_responses.py Outdated Show resolved Hide resolved
tests/system/responses/test_succesful_async_responses.py Outdated Show resolved Hide resolved
umodbus/client/serial/rtu.py Outdated Show resolved Hide resolved
@tiagocoutinho
Copy link
Contributor Author

Hi @jaapz,
Thanks for your review.

I am in favour of moving the async_send_message() to it's own module and renaming the function to send_message().
As you probably noticed, I there is no import asyncio in the PR. This is intentional and allows umodbus to state that is is async library agnostic.
The idea was that it would be simple to use umodbus in an asyncio application and, by writing a simple adapter, possible to use in a curio, trio or anyio application.

So, long story short, what do you thing about naming the module umodbus.client.tcp.asynch instead?

@jaapz
Copy link
Member

jaapz commented Nov 16, 2020

@tiagocoutinho that makes complete sense, calling it umodbus.client.tcp.async instead is fine by me. Maybe you could also add a bit of documentation to the readme stating that we are in fact async library agnostic?

@tiagocoutinho
Copy link
Contributor Author

Maybe you could also add a bit of documentation to the readme stating that we are in fact async library agnostic?

You read my mind :-)

@tiagocoutinho
Copy link
Contributor Author

tiagocoutinho commented Nov 16, 2020

I think it should be ready for review now.
I re-based from master in order to update the examples with the argument parser.
I took the liberty of adding also a reference to gevent in the README and I added a curio tcp client example.

Copy link
Member

@jaapz jaapz left a comment

Choose a reason for hiding this comment

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

Just a few small changes/comments, almost there! Again, great work, thanks for your contributions and thanks for bearing with me during the review.

README.rst Outdated Show resolved Hide resolved
umodbus/client/tcp/__init__.py Outdated Show resolved Hide resolved
tests/unit/test_tcp.py Outdated Show resolved Hide resolved
umodbus/client/tcp/asynch.py Show resolved Hide resolved
@jaapz
Copy link
Member

jaapz commented Nov 17, 2020

I've done another small review, everything else looks good to me. Maybe @OrangeTux can take over from here to test it a bit and get this merged and released?

@tiagocoutinho
Copy link
Contributor Author

Hi @OrangeTux , @jaapz,
Just pinging... Does everything look ok with the PR?
I don't see any pending changes. Please let me know if you are waiting on me.
Thanks

@jaapz
Copy link
Member

jaapz commented Dec 14, 2020

Hey @tiagocoutinho, I just need some time to properly test this before merging. Sorry for the wait, I'm pretty busy at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants