-
Notifications
You must be signed in to change notification settings - Fork 956
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
Redesign Async client implementation with support to tornado and asyncio #246
Conversation
* updating lots of reference documentation * fixing the fifo semantics of serial clients * using transaction manager in async clients * fixing references
- fixed messages not passing **kwargs to base - fixed binary framer off by 1 - fixed mei_message rtu size tests - added a message generator to use with message parser - fixed message parser with ascii - tested message parser with all formats (added to messages)
Feature server address
* allow codes like payload builder to encode * added IPayloadBuilder interface (future) * renamed builder methods to reflect vision * added error code decoding to name * fixed affected tests
- moving custom datastores to examples - bumping required versions - making the debug server console optional - updating documentation
Thanks for your work on this. It is much appreciated! |
@cbergmiller the idea was to handle all the intricacies from with in the pymodbus library and provide the user a simple interface and that is the reason why the event loop is run beforehand, if you check the other factories we are following the same pattern. Your question is valid but I would consider it as an enhancement on top of the proposed change. I hope I have answered your question, if you think something could be improved please feel free to raise a PR/issue. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, I can't wait for this to be merged to mainstream.
""" | ||
self._dataReceived(data) | ||
|
||
def create_future(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How using this helper is better than using normal asyncio interface? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pax0r didn't get your question, could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it's a good idea to create helper function which consists only of single call to some standard lib element. I believe it may be better to just directly use asyncio.Future()
. But it's a nit, you can ignore it ;)
""" | ||
self._dataReceived(data) | ||
|
||
def create_future(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it's a good idea to create helper function which consists only of single call to some standard lib element. I believe it may be better to just directly use asyncio.Future()
. But it's a nit, you can ignore it ;)
_logger.debug('Connecting.') | ||
try: | ||
yield from self.loop.create_connection(self._create_protocol) | ||
_logger.info('Connected to %s:%s.' % (self.host, self.port)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class does not have self.host
(and host
don't make sense for serial connection).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pax0r thanks for taking the time to review , much appreciated. Lot of cleanup is still to be done with this branch. I am going to keep this PR for some more days for others to review as well and will take care of your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhoomakethu, I just started to work on project using asyncio and modbus communication and decided to give a try to your work. For now looks like asyncio part is working well, I'll come back if I will found any more problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks , just curious, are you dealing with the serial part or the TCP? Please raise an issue if you come across any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now Serial only for communication with Fatek PLCs, maybe I'll use TCP later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently did some work on the Serial Asyncio Modbus Client. The API of serial_asyncio.create_serial_connection
is different from the socket API. Also, the serial kwargs (baudrate etc.) were missing in AsyncioModbusSerialClient.__init__
Method. I didn't have time to test my changes because serial_asyncio
does not yet work on windows. I will generate a pull request anyway for reference.
[FIX] usage of create_serial_connection
[FIX] testSerialAsyncioClient
2. Fix/update unit tests
Fix AsyncioModbusSerialClient (serial options and create_serial_connection usage)
Will be merging this PR to |
@dhoomakethu TBH I'm not quite sure how to implement this, since I'm using already using my own asyncio loop in my application. Since It's using Task exception was never retrieved
future: <Task finished coro=<BAS.async_start() done, defined at /srv/bas/lib/python3.5/site-packages/bas/core.py:70> exception=RuntimeError('This event loop is already running',)>
Traceback (most recent call last):
File "/usr/lib/python3.5/asyncio/tasks.py", line 239, in _step
result = coro.send(None)
File "/srv/bas/lib/python3.5/site-packages/bas/core.py", line 72, in async_start
await neuron.switch_to_async(self.loop, self.basConfig.alias_dict)
File "/srv/bas/lib/python3.5/site-packages/bas/brain.py", line 141, in createClient
loopi, clienti = ModbusClient(schedulers.ASYNC_IO, host=self.modbus_server, port=self.modbus_port, loop=self.loop)
File "/srv/bas/lib/python3.5/site-packages/pymodbus/client/async/tcp.py", line 45, in __new__
timeout=timeout, **kwargs)
File "/srv/bas/lib/python3.5/site-packages/pymodbus/client/async/factory/tcp.py", line 97, in async_io_factory
client = loop.run_until_complete(asyncio.gather(cor))[0]
File "/usr/lib/python3.5/asyncio/base_events.py", line 454, in run_until_complete
self.run_forever()
File "/usr/lib/python3.5/asyncio/base_events.py", line 408, in run_forever
raise RuntimeError('This event loop is already running')
RuntimeError: This event loop is already running Or am I doing something wrong? I'm quite new to asyncio, so please tell me. |
@makuser you don't have to use the factory. You can import the async client directly async def task(loop):
client = AsyncioModbusTcpClient(loop=loop, host='127.0.0.1')
await client.connect()
rr = await client.protocol.read_holding_registers(1, 2, unit=1)
print(rr) |
@makuser, Can you provide a code snippet on how exactly you are trying to create the client ? Just in case you are not aware there are examples to create and use the various clients here, more specifically asyncio client example is here. If you want to use existing loop that is already running, pass it as a kwarg Update: The above scenario works when the asyncio loop is not yet started, in case you need to use the existing loop , please try what @cbergmiller has suggested. |
@dhoomakethu I think what @makuser is seeing is a genuine issue with the factory implementation. Ideally a new event loop should be created in the factory using |
Pymodbus async client now supports Tornado and asyncio along with twisted for async client. The user can choose the backend as per his needs. Sample examples as below.
More detailed examples are available here
Note: asyncio implementation is still being ported frompython3
branch and will be available only for python3@moltob and @madsl I have used your asyncio code for tcp/udp , Please feel free to give a look. The unit tests are yet to be updated.